Skip to content
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

[Style dropdown] Improvements #14274

Open
5 of 13 tasks
Witoso opened this issue May 29, 2023 · 5 comments
Open
5 of 13 tasks

[Style dropdown] Improvements #14274

Witoso opened this issue May 29, 2023 · 5 comments
Labels
Epic package:style squad:core Issue to be handled by the Core team.

Comments

@Witoso Witoso added squad:core Issue to be handled by the Core team. Epic labels May 29, 2023
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jun 1, 2023
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Jul 21, 2023
@wimleers
Copy link

wimleers commented Oct 3, 2023

Drupal uses the official Image plugin, but also has its own DrupalMedia plugin. This also uses a (block) widget.

What would allow Style to work for widgets? Because that is hard-blocking https://www.drupal.org/project/drupal/issues/3117172. I thought #11577 was the issue to watch, but it was closed 4 days ago: #11577 (comment). I'm confused 😅

@Witoso
Copy link
Member Author

Witoso commented Oct 3, 2023

Thanks! Allow me to share more comments on this (@Reinmar or @niegowski feel free to share yours). The original issue was too generic, and it lacked the very specific detail that should have been there: support for the current block widgets of the editor (tables, lists, etc.). Most of those were implemented, hence the closing.

Style dropdown [SD] operates on HTML alongside the GHS plugin. We cannot make it generic in a way, that it works with all widgets by default. Widgets are a layer SD isn't concerned about.

What we need to do is to make SD aware how it should integrate with certain features (and their HTML). That's why we need more specific feedback, which features lack such behavior.

And then, there are tricky parts. Images have their styling implementation. Should we duplicate it? Combine it with SD? Those are questions we don't have answers to yet.

Drupal uses the official Image plugin, but also has its own DrupalMedia plugin. This also uses a (block) widget.

@niegowski any ideas how this could be done?

@wimleers
Copy link

wimleers commented Oct 3, 2023

Most of those were implemented, hence the closing.

Most (table, list), but not all. Image in particular is very often requested.

Images have their styling implementation. Should we duplicate it? Combine it with SD? Those are questions we don't have answers to yet.

Ahhh! This is why Image is not supported!

ImageStyle does far more than just adding a class attribute. AFAICT the only desire is the ability to add a class attribute, without changing from block to inline or vice versa. So, I think this approach would make sense:

  1. ImageStyle continues to work as it does today
  2. Style plugin gains the ability to add a class attribute value to a selected image.
  3. For example, say we define a "Vintage photo" (class name: vintage) style for <img>, which applies some CSS to do filtering.
  4. If I select a right-aligned image (alignBlockRight) and click the "Vintage photo" style, it would add to the existing <img class="image-style-align-block-right"> and hence change it to <img class="image-style-align-block-right vintage">
  5. Upon removing a style: same thing.

IOW:

  • "image style" is for image positioning, size, etc. These are all deeply tied to the image and hence appear on the image balloon only.
  • Style for an <img> is for adding classes, just like for all other elements, and is virtually always presentational.

We cannot make it generic in a way […]
I understand that. I'm not asking for it to work everywhere, "magically". I'm only asking for an API to allow other plugins to support it.
Since you've done it for Table etc, it must be possible. But there's no public API — or if there is, it's missing from https://ckeditor.com/docs/ckeditor5/latest/features/style.html#common-api (really hoping it's "just" a documentation problem! 😄 🤓).

@Witoso
Copy link
Member Author

Witoso commented Oct 4, 2023

Image in particular is very often requested.

Could you share some links to remarks about this? It would help a lot. I checked most conversations about the Style dropdown on our GH, and the tables and lists are the top comments, images almost do not appear. Even on the Drupal side img was mentioned once.

So, I think this approach would make sense:

Lots of those comments make sense 👍 I just keep in the back of my head the remarks on the need for SD to be visible directly on the elements as the better UX choice than the large list of styles. I'm also worried about the confusion of users that applying image styles would be in two places and discovering which is where. I think most people won't grasp the difference between the presentational/positional. cc @dagdzi

Since you've done it for Table etc, it must be possible.

Quick look at the code, it is. The new plugin was created to listen to the SD events to inform if the style can be applied.

@bkosborne
Copy link

I'm trying to make a custom block widget I created compatible with the Styles plugin. It's view downcast creates an outer <section> element that wraps a title and description div. It's very similar to the output that the tutorial for creating a block widget produces.

I'm trying to figure out if it's possible for me to allow creating a Style for the outer section element. In CKE4 I could do this by declaring the widget properly.

From reading the comments above, it seems like it may be the Style plugin that needs to be made compatible with my custom element, is that right? Does that mean I can't really do it without hacking the Style plugin?

Please bear with me - the linked example for how the Table plugin was made to work with this didn't help me much. For example, it doesn't have anything there about data downcast where I might need to concerned about combining standard CSS classes on the outer element with classes from the Style plugin. Or maybe I don't need to deal with that manually?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic package:style squad:core Issue to be handled by the Core team.
Projects
None yet
Development

No branches or pull requests

5 participants