-
Notifications
You must be signed in to change notification settings - Fork 40
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: Allow the Styles dropdown to use tag names (for easy use of the <small> tag) #2980
Comments
As a |
But the Styles dropdown is for adding Styles (classes) not for adding tags, no? Maybe what we should use is a button (like Bold and Italic?) Should we look at how other editors handle this? |
The styles dropdown can actually provide both elements and classes. Per the help text:
Unfortunately, I found it does not allow an element type only, you get a validation error if providing a style like Though the Seems like supporting an element without a class might solve this problem entirely. |
I filed a PR at backdrop/backdrop#2102 that allows the styles dropdown to include a tag name without a style. It looks like CKEditor supports tag names without classes just fine. Besides "small" tags, there are a number of other inline tags that CKEditor does not provide dedicated buttons for, including:
So any of these tags would be possible support via the "Styles" button. I also found that the Styles button includes 100% of the functionality of the "Format" button. The Styles button handles both inline and block styles, while the Format button only handles block styles. Perhaps as a follow-up issue, we should attempt to remove or deprecate the "Format" button and use the "Styles" button exclusively. |
from an editor [UX] perspective, I love the idea of combining the two drop-downs. It's hard for people to know the difference, and if Styles can do everything Format can do, I don't see any good reason to keep them both. See #2983 I also really like this approach from a site-architect [UX] perspective. When using the Format drop-down I always wonder what I need to do to add another element to that list. It's not intuitive that adding the tag under allowed HTML tags is the way to have it appear. By switching to styles, and providing the area where we can define those styles, it becomes much clearer to how to set up which items appear in the list. |
I've tested the PR in the sandbox, tried it with Added milestone 1.12.0. |
👍 |
PR reviewed and tested (backdrop/backdrop#2102 (comment)). LGTM |
...there is a small problem in the code of the PR where multiple spaces have been added between the closing bracket of an if statement and the opening curly bracket. Also, can we please refresh the PR sandbox @quicksketch (the PR was opened a year ago, so we had a couple of releases since then + CKEditor has been updated with #3355)? Besides, I would love to test this using our new fancy Options element (#1005) 😉 |
...also, unfortunately this has missed the train for 1.12 😞 (feature freeze was 3 days ago). |
Made a new PR based on @quicksketch's one, addressing the comments made there: backdrop/backdrop#2777 |
@BWPanda Quickly tried the PR in the sandbox website: I've added these tags (without class) to the Style list:
I was able to use all of the tags from the Style dropdown after adding |
I've updated my PR with a change to the Style List description (added a note about making sure tags are added to allowed list if the HTML tag filter is enabled). |
Better UX I think to automatically include these tags in the allowed tags list rather than requiring to re-write them there. |
I've updated the PR based on @docwilmot's suggestion. Any tags added to the Styles list are now automatically added to the Allowed HTML tags list as well. I also updated the description text appropriately. |
Thanks @BWPanda for working on this and the updated PR! I left a comment on your MR that server-side validation would work, but we have existing client-side tools to help with this exact problem of adding/removing tags from the filter list, we should use that. backdrop/backdrop#2777 (review) For example dragging the Strikethrough button gives a message like this: This also gives the opportunity for other filter modules to respond to the adding/removing of tags. This JS is a little heady though... I'll pull in your changes and then add the client-side work to my PR. |
New PR up that adds the tags client-side instead of server-side, to match that of how the toolbar buttons work. |
@quicksketch I tried the new PR. It partially works.
So for some reason the automatic part of adding to Allowed HTML tags isn't working. Update: there was some combo where this worked. But mostly not and when I try to recreate that combo I can't get it to work again. So that JS seems a bit fragile. |
I tested this in my own sandbox. Is the 'Allowed HTML tags' field supposed to be updated as soon as I enter something in the 'Styles' field? If so, didn't work. Also didn't work when saving. Not sure if it's the PR or my understanding... |
Also for me, the PR doesn't make it so that the "Allowed HTML tags" get automatically updated with values from the Styles field. @BWPanda I guess they're supposed to be added when they have been entered, at least that's how the toolbar buttons work. |
Describe your issue or idea
I often want to make text in various text areas smaller. Unfortunately Backdrop's implementation of ckeditor is lacking the ability to do so :(
From stack overflow: https://stackoverflow.com/questions/24171606/ckeditor-format-tags-and-a-custom-small-tag
Search for CKEDITOR.config.format_h6 in ckeditor.js and add CKEDITOR.config.format_small={element:"small"};.
open the languagefile you want (e.g. en.js) and add "tag_small":"small text".
now CKEditor supports the small tag.
Steps to reproduce (if reporting a bug)
Attempt to wrap some text in the
<small>
tag.Actual behavior (if reporting a bug)
There's no
<small>
tag.Expected behavior (if reporting a bug)
A
<small>
tag.PR by @quicksketch: backdrop/backdrop#2102PR by @BWPanda: backdrop/backdrop#2777PR by @quicksketch: backdrop/backdrop#2799
The text was updated successfully, but these errors were encountered: