Allow defining customized manual and/or automatic link decorators

[msamsel]

Suggested merge commit message (convention)

Feature: Allow on defining customized manual and/or automatic link decorators. Closes ckeditor/ckeditor5#4829 .

---

Additional information

Required theme-lark PR t/ckeditor5-link/186

• Currently you cannot use both option at the same time when those works on the same attribute.

[msamsel]

There still remain documentation guide to write.
And currently it's pretty simple UI, maybe we could invent some better approach for that.

[mlewand]

@msamsel what is the reason for graying out 2 options? Is it that these are the options set up automatically? If so I think it is safe to skip them in the UI and make them only happen behind the scenes.

@dkonopka could you have a look at this from UI/UX point of view? Though the components are aligned with our design in terms of styles I feel that this proposal makes the dialog very heavy. IMO it doesn't look nice if there are more than 2 options. Part of which might be displaying these grayed out options.

[msamsel]

@msamsel what is the reason for graying out 2 options? Is it that these are the options set up automatically? If so I think it is safe to skip them in the UI and make them only happen behind the scenes.

I haven't configure it and it is just default behavior. As this is SwitchButton instance its just indicate that this button is turned on. And gets highlight like bold button or any other regular button.

But it might be somehow improved I see that options for table are not highlighted, so there should be some way to not showing it up.

[mlewand]

So definitely we need to unify that, my first impression was that the last two options were simply disabled. Whereas as I understood from your previous comment it means that the first option is hovered (thus it's white) and others are in their default state (gray background), right?

[msamsel]

@mlewand unfortunately couldn't get mouse pointer on this screenshot. Options have this state:

1. has no interaction
2. is active (turned on)
3. is hovered

There is manual test, which has an example of manual decorators. You should be able to get live demo with this menu if you need check more details.

[msamsel]

@mlewand it's appear that feature that switchbutton doesn't have background inside list is available in CKE5:
https://github.com/ckeditor/ckeditor5-theme-lark/blob/d3e4266526b277a7c1e73da91f8e5c8ddf38e7df/theme/ckeditor5-ui/components/list/list.css#L64-L74

So I just give proper classes to my component and import additional style to manual test, and everything is working nicely without additional hacks.

[msamsel]

@mlewand now grey background is not present for active switch buttons. I reuse class names already available in our theme.

[mlewand]

The UX of ths feature needs to be polished.

• Switches should apply the change the moment user clicks it, no clicking accept button should be required.
• I think that the toggle buttons provided by this feature should also be visible on a first link view (where you have clickable link). I find it really more difficult that it should be to get the job done when options are hidden in second view.

As a reference, a gif from the current implementation:

RFC

Should links be applied to the "link preview" visible on the first view? I'm thinking mostly about the download attribute here. It would be nice if the link gets the [download] attribute. Another useful attribute might be [rel="noreferrer"].

But at the same time I find it useful only for these particular attributes. Other, like HTML class doesn't make much sense in this regard.

[msamsel]

Should links be applied to the "link preview" visible on the first view?

I have one doubt about it. When someone creates new link, by default it's opened form view. So in this case it will extend his path. He will have to create new link and then close edition to go to action menu and apply proper decorators. It sounds wrong for me.

Another case is that those menus are grouped by function: "action" and "edititng". Providing new decorator to link is editing from my perspective.

[poppaul95]

@msamsel can you provide some eta for when this feature will be available?

[dkonopka]

I think that the toggle buttons provided by this feature should also be visible on a first link view (where you have a clickable link)

I know it's handy to have all need things at the first view, but I can't agree with you.

It's a 2-step form (preview & editing), so we should teach users if they want to change anything they should go to the "edit" section.

OTOH, I'm not so sure about "switch" buttons - it looks good as a single option (for tables).

I'm wondering, maybe we should consider a new UI element - a checkbox? We definitely need an opinion from @oleq here, but at this moment we can live with those switches?

[msamsel]

@poppaul95 I'm sorry there's no precise ETA yet. Initially we were looking to include it in iteration 24th, however we already know that we'll have to put it into iteration 25.

The best way to stay up to date is to subscribe ckeditor/ckeditor5-link#186 issue as we'll update it as the feature lands in the master branch.

[mlewand]

Interesting point @dkonopka. I think that in this case we should wait to hear more voices on the design.

I'm still not convinced whether we should put these toggles in the main view (or both) or in the second.

Talking about two view approach, I see couple of problems with this idea:

• Accept/cancel buttons position
• As a user I'm not feeling comfortable with a thought of changing target attribute in more than one link in my document.
• Closing confirmation

OTOH, I'm not so sure about "switch" buttons - it looks good as a single option (for tables).

This should not be our concern at this point, we're giving this option to the developers to see whether this feature will be used.

Accept/cancel buttons position

With new inputs added below, I believe that the ok/cancel buttons should be moved to the very bottom of the balloon. I mean we could go unusual here, but it's a common solution to put action buttons at the bottom of the dropdown if there are multiple options in it.

Closing confirmation

My initial impression was that attribute gets applied the moment I toggle it. So I just did that and moved on. Later I was surprised that my changes were not applied.

For that reasons apps typically put some sort of confirmation when you're closing "dirty" dropdown/dialog. This would have tipped me of this behavior.

[msamsel]

My initial impression was that attribute gets applied the moment I toggle it. So I just did that and moved on. Later I was surprised that my changes were not applied.

That's true. Currently entire view works as a form, you need to submit changes with ok button, to apply changes. It's made to be perfectly integrate with "link" command, as it was one of point discussed here:
https://github.com/ckeditor/ckeditor5-link/issues/186#issuecomment-483257606

So now all link attributes (href and custom decorators) are passed inside link command.

[msamsel]

I've rebased this PR to the current master to fix an appearing conflicts.

[msamsel]

The new design looks pretty nice. I'm only concerned about

Closing confirmation

That I mentioned in #222 (comment) - but if it worries us that's totally something that we should push to a follow-up issue.

I feel like introducing such feature might take some time. If I understand correctly we should implement some sort of feedback for the user that current state is "dirty" and confirm if he really want to close edition.
I think that best option could be implementing new feature to notification which utilize window.confirm in a similar way as it is done now for warnings. That's why maybe we could implement it as further improvements ticket.

@oleq, @mlewand WDYT?

[msamsel]

You fixed a bug. Would be good to have a test for that in order to don't fix the same problem in the future.

Thx for feedback. I added unit tests for this change now.

[oleq]

is. Also most likely isExternal because we use camel case for props in the project.

[oleq]

But... you didn't change the configuration documentation, did you @msamsel?

[msamsel]

But... you didn't change the configuration documentation, did you @msamsel?

I'm still in progress. Right now I just implement new config and correct tests and now switch to docs.

[oleq]

How is this supposed to work if ckeditor5-link/theme/linkform.css defines this class with display: block (not flex?) https://github.com/ckeditor/ckeditor5-theme-lark/pull/233/files#diff-e9ba6bcf18c57cfa9c06993af81ac820R54

[msamsel]

@oleq I see that @dkonopka made requested changes.