New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UX] CKEditor - Add CSS class option in link dialog #3307

Closed
olafgrabienski opened this Issue Oct 10, 2018 · 19 comments

Comments

@olafgrabienski
Copy link

olafgrabienski commented Oct 10, 2018

There are currently two ways to add a link class to a body or to a comparable field which works with CKEditor:

  1. Switch to source code and add class(es) in the HTML manually.
  2. Enable the Style dropdown in the editor settings and provide link classes for the dropdown to let a person choose one of it.

The source code way is very flexible, but we can't expect all people to be comfortable with it. The Style dropdown solves this limitation potentially, but it has two drawbacks:

  • It allows only the selection of one style.
  • The handling is annoying: To use an object style on a "special selectable object" like a link in CKEditor, you have to select the whole object before, but it's hard to select the whole link: Selecting it gradually by mouse or arrow keys doesn't work; the Style option becomes only available when you doubleclick the link. But before, unfortunately in this case, the double click opens the link dialog. So a person has to close the dialog, and only then he or she can finally add the link class.
  • Update regarding the handling: There's at least one case where it's not possible to select a link. When the link surrounds an image, only the image is selectable.

To not make it so difficult to add a link class, I suggest to provide a CSS classes input for it.

screen shot 2018-12-20 at 7 25 41 pm


PR: backdrop/backdrop#2428

@olafgrabienski olafgrabienski changed the title [UX] CKEditor - Add class option in link dialog [UX] CKEditor - Add CSS class option in link dialog Oct 10, 2018

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Oct 10, 2018

Related issues:

  • Port D8 Editor Advanced link, merge in core #1777
  • CKEditor - need to use with Colorbox #1580
@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Oct 10, 2018

In theory, this feature request could be solved by the related issues mentioned above. It seems however that these were too ambitious (and partially controversial) to move on. And proposals to provide link attribute related features via contrib modules, led to no results so far.

In my opinion, the option to use link classes is a quite common use case (think of Bootstrap button classes, for instance). On the one hand, link classes could be a good point to continue work on the integration of link attributes (be it core or contrib). On the other hand I'd like to make the use of link classes as easy as possible for people on current Backdrop projects, so I'm looking for not too long-term solution a.k.a hoping on the 'baby steps' approach.

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 20, 2018

Recently I found out that it's not only difficult but in some cases impossible to add a class to a link via the Style dropdown (have updated the issue description respectively), which motivated me to have a look at this issue again. Doing so, I found an interesting conversation in the contrib queue:

In backdrop-ops/contrib#227 (comment) @BWPanda suggested "the ability to add classes to links" to be "a core feature", and @jenlampton reacted with a 👍 .

My impression is that many people would like this feature, including me, and I'd love to help with it. Is anyone up to make a start?

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 21, 2018

okay, so this turned out to be very easy to implement. PR up for review!

@BWPanda

This comment has been minimized.

Copy link

BWPanda commented Dec 21, 2018

Works for me! Very nice.

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 21, 2018

Great, @jenlampton, thanks! The PR works for me as well (tested in the sandbox site).

At the moment it's however possible to provide classes with invalid characters like my/class, another#class, or example$class, and they are rendered. I guess we need to use backdrop_clean_css_identifier to prevent that. (See also the related issue #3428).

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 21, 2018

Forgot to change status to „needs work“.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 21, 2018

Alright, I've added a validation handler that will clean up the CSS classes. PR updated, new sandbox should be generating now.

@BWPanda

This comment has been minimized.

Copy link

BWPanda commented Dec 21, 2018

Changes work for me. Tried class of 'css/is*awesome', was correctly cleaned to 'css-isawesome'.

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 21, 2018

Yes, it works for me as well!

I'm not sure about underscores: In this PR they are changed to hyphens at the moment which shouldn't be the case because underscores are valid characters. AFAIK it happens because #3428 isn't ready, so I think in this issue we can't change the behavior. Can somebody verify my assumption?

I've also had a look at the wording regarding classes at other places. In this PR it's "CSS classes", in Blocks they are called "Block classes", and in the Views fields Styling section it's "CSS class" again (but singular). In my opinion, "CSS classes" is the best. If we want to harmonize the wording, we should file follow-up issues for Blocks and Views.

@jenlampton

This comment has been minimized.

Copy link
Member

jenlampton commented Dec 22, 2018

I'm not sure about underscores

I think we should implement the same solution here as we did in https://github.com/backdrop/backdrop/pull/2411/files.

I've pushed an update to the PR that should leave underscores alone :)
backdrop/backdrop#2428

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 23, 2018

Perfect, underscores don't change to hyphens anymore! RTBC, in my opinion (without having a close look at the code)!

As a last sandbox test, I've enabled the Style dropdown in the "Filtered HTML" format to check if link styles provided by the new CSS classes option appear also in the dropdown if they match a class of the predefined Style list. That's the case. Also, classes provided by the Style dropdown appear in the CSS classes input. So, both ways to provide CSS classes for links are compatible, which is great!

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Dec 28, 2018

Implementation looks great.

Should this be added to core instead of leaving to contrib Seems like there could be several other things added to this dialog. Or are classes common enough that we think it should be shown to nearly all site users? It seems reasonable but @olafgrabienski so far is the only one saying this should be done.

Looking a little further, I see we also have #1777 which adds not only class but also id and and rel. So perhaps any questions about further features should go there. For now I think the question is simply: should we put this smaller feature in right now?

@stpaultim

This comment has been minimized.

Copy link
Member

stpaultim commented Dec 28, 2018

I kinda like this idea, but am sympathetic with the idea that it MIGHT not be necessary. The problem I run into fairly often is classes entered into the body field getting lost or filtered out. I'm not sure that this helps with that problem at all.

I think that most people who know enough to add a class to a link are probably also able to edit the source code and put it in there. Having said that, I am not objecting to this feature. I think I would use it, but I'm not sure it really solves my most common problem with classes in the body field.

I'm updating my comments to vote in favor of this feature. I've tested the pull request and I like it and think I'll use it.

@klonos

This comment has been minimized.

Copy link
Member

klonos commented Dec 28, 2018

I also find this very useful 👍...if we end up implementing #1777 in core (which I think we should), then we can move all these settings inside an "Advanced settings" fieldset to keep the dialog as clean and as less intimidating as possible, while at the same time offering flexibility for site builders and more advanced content authors.

@olafgrabienski

This comment has been minimized.

Copy link

olafgrabienski commented Dec 29, 2018

@olafgrabienski so far is the only one saying this should be done.

@quicksketch, my impression was that also @BWPanda and @jenlampton support the idea to add this feature to core. Apart of that, I see your concern to show CSS classes "to nearly all site users", and I guess it makes sense to move such features in a collapsed "Advanced settings" section. (Actually, we could do that already with the current options, as there is also the "Open in new window" checkbox on the link dialog, so they would be already two settings to put in that section.)

I'm still voting to "put this smaller feature in right now" because it's not contrary to #1777 but a good start.

@laryn

This comment has been minimized.

Copy link
Contributor

laryn commented Dec 29, 2018

I like this as a smaller easy addition now and also this and other settings into "advanced settings" in the future.

@BWPanda

This comment has been minimized.

Copy link

BWPanda commented Dec 29, 2018

Yes, I think this would be a useful addition to core.

@quicksketch

This comment has been minimized.

Copy link
Member

quicksketch commented Dec 31, 2018

Sounds good to me, thanks everyone for your thoughtful responses. I've merged backdrop/backdrop#2428 into 1.x for 1.12.0. Thanks @jenlampton, @olafgrabienski, and @BWPanda for providing testing and the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment