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

Move block indentation (p.indent1/2/3) functionality to contrib #107

Closed
quicksketch opened this issue Nov 28, 2023 · 11 comments · Fixed by #111
Closed

Move block indentation (p.indent1/2/3) functionality to contrib #107

quicksketch opened this issue Nov 28, 2023 · 11 comments · Fixed by #111

Comments

@quicksketch
Copy link
Member

CKEditor 5 uses indent1/indent2/indent3 classes when indenting content with indent / de-dent buttons. Backdrop does not currently provide any styling for these classes, despite it being the same classes that CKE4 used (apparently we did not make these buttons available in CKE4). Alternatively, we could remove the indent/de-dent buttons in the out-of-box buttons.

@indigoxela
Copy link
Member

indigoxela commented Nov 28, 2023

That's an interesting question... Indent/outdent in CKE is a context-sensitive thing.

For v4 the plugins for block and list indent/outdent are provided by a contrib module, v5 incorporated it in core, it seems. (Or, we did that...)

Now for the context:

The indent/outdent functionality is pretty useful for nested lists. Without it it's not really possible to create list nesting via UI.
It's however a bit odd with block content. Manual indent for div or any other block level element is hardly ever mobile-friendly. But as CKE probably won't let us limit the functionality to only lists, we might end up providing the styles. Not sure.

@quicksketch
Copy link
Member Author

For v4 the plugins for block and list indent/outdent are provided by a contrib module

I did not realize that. It would be nice to have CKE5 support/migrate this functionality from the Backdrop contrib module if we're going to include the indent/de-dent buttons in core.

Weirdly, the ckeditor.module (v4) already includes this line to specify the indent classes:

'indentClasses' => array('indent1', 'indent2', 'indent3'),

But it does not include any kind of styling (nor buttons).

These are the same default indent classes as CKE5. Some users coming from Drupal 7 have reported a similar problem in D10 (from this Slack thread).

@wimleers
Copy link

wimleers commented Dec 1, 2023

These are the same default indent classes as CKE5. Some users coming from Drupal 7 have reported a similar problem in D10 (from this Slack thread).

You beat me to it! 😄

@wimleers
Copy link

wimleers commented Dec 1, 2023

Also, interesting that you enabled the IndentBlock plugin out-of-the-box. Because CKEditor 4 in Drupal did not do that.

@indigoxela
Copy link
Member

indigoxela commented Dec 1, 2023

We might be able to disable (or actually not load) the plugin in function ckeditor5_get_config().

A simple $config['removePlugins'] = array('IndentBlock'); seems to do the trick.
If we do that before calling backdrop_alter('ckeditor5_config'... a contrib or custom module would still be able to load and use it.

Do we want that?

The Backdrop module (ckeditor_indent) seems to enable it by default. People upgrading from 4 to 5 might miss functionality if we don't.
Or should we disable all indent plugins by default and leave enabling to contrib? It's part of our build.

@quicksketch
Copy link
Member Author

Well just disabling the plugin would certainly solve the problem. I must have just assumed that we had indent/dedent enabled since the indent class list is present in our core ckeditor.module.

@indigoxela what do you think? Should we remove it from the build? Or is it a dependency for some other functionality?

Drupal 11 seems to include the plugin in the build but it's not actually enabled. The Drupal contrib project ckeditor_indentblock simply enables it and adds a CSS file.

@quicksketch quicksketch added the type - question Further information is requested label Dec 1, 2023
@olafgrabienski
Copy link
Member

Not sure what exactly this issue is about: indentation for lists or in general? I think, for lists it's quite important to have the option.

@indigoxela
Copy link
Member

indigoxela commented Dec 2, 2023

Not sure what exactly this issue is about

The current discussion is about IndentBlock - which adds CSS classes to block elements like paragraphs, which can then get styled as indented via CSS.

To my understanding we agree, that list indent (nesting of ul/ol) is a useful feature. We're not so sure about block elements.

@indigoxela
Copy link
Member

I must have just assumed that we had indent/dedent enabled since the indent class list is present in our core ckeditor.module.

Sure, it's enabled as it's declared as plugin_dependency.

We have several options now:

  1. enable by default, but disable via removePlugins config (that makes it very easy for contrib to enable)
  2. drop that dependency (but then it's useless without the List or DocumentList plugin)
  3. switch that dependency to DocumentList (didn't test the builder behavior with that)

what do you think? Should we remove it from the build?

That could make it harder for contrib to add it back, I guess.

I really like the feature for lists, but I'm no fan of the block indent feature. But that's really only a personal preference, no actual recommendation.

As the feature is on in the v4 module, we can't know, what people actually prefer (list / block). We can only assume. The only thing we currently know is, that there are 88 installs - those people were able to find that plugin.

@indigoxela
Copy link
Member

Here's my suggestion: #111

It switches the plugin_dependency from IndentBlock to DocumentList, which seems to make more sense to me.

Contrib can still enable the plugin (it's part of @ckeditor/ckeditor5-indent anyway). That shouldn't be too hard, as it can be done purely via hook_ckeditor5_config_alter (I think - didn't test).

@quicksketch
Copy link
Member Author

Thanks @indigoxela, I think that should do perfectly for now. We'll leave the contrib module to handle indenting blocks if necessary.

@quicksketch quicksketch changed the title Add styling for indentation Move block indentation (p.indent1/2/3) functionality to contrib Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants