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

State of the plugin #1

Closed
PRGfx opened this issue Mar 28, 2022 · 1 comment
Closed

State of the plugin #1

PRGfx opened this issue Mar 28, 2022 · 1 comment
Labels
question Further information is requested

Comments

@PRGfx
Copy link
Contributor

PRGfx commented Mar 28, 2022

I looked into enabling the options for ordered lists, but I came across some things I don't want to touch before I am sure about the intentions behind them.

  • list-style-type attribute
    This plugin uses an additional attribute on the list. This results in invalid HTML. Rather it should apply e.g. a data-list-style attribute.

  • list-style as attribute
    I assume this plugin uses an additional attribute rather than the CSS property (as done by the ckeditor module) to allow for more flexibility with custom properties that are not valid CSS?

  • class names
    This plugin describes list-type presets with both title and valuevalue being

    the value of the list-style-type attribute of the list.

    That however is not the case. Instead the id/key of the preset is used. In fact as far as I can tell, that value is used to upcast an existing class name into the preset ID (and is then lost from the element). That's a bit confusing I think. Is this deliberate? I would have expected the preset value in the view.

  • breaking lists
    The list editing was pretty buggy for me, unfortunately, as the lists got split quite frequently even when selecting all content. In some parts, that I see connected to this issue, the plugin differs from the "original" ckeditor module. Is there a reason to this (i.e. to deliberately customize the "default" behavior)? I haven't checked, whether the ckeditor module has changed since the last changes here.

Apart from that there was a minor bug with the list-style menu staying open if I leave the element, while the button to close the menu is gone. This could be quickfixed with something like that on the button:

isOpen() {
	return this.shouldDisplayAdditionalButtons() && this.state.isOpen;
}
@paxuclus
Copy link
Owner

Hi @PRGfx 👋 ,

  • list-style-type attribute: I agree, but see next point
  • list-style attribute instead of style: Initially, I wanted to use CSS classes to have a clean and flexible way of styling lists (in case list-style-type is not enough), but I had trouble getting classes to work with CKE5. I then used the next best thing which was the list-style-type attribute.
  • class names: You are absolutely right. That was not intentional and is indeed misleading. I agree that the value should be used instead.
  • breaking lists: Yes, the editing experience is quite fiddly. As fas as I can remember I wanted to use the default CKE5 plugin but the list style feature was introduced in v22 which was newer than the CKE5 version used in the Neos UI (still v16?). I then tried to load the CKE5 plugin in a newer version but that caused two instances of ckeditor to spawn (hence the custom webpack.config.js).

The isOpen() bugfix makes sense 👍 .

I believe the right way to do this would be to upgrade CKEditor in the Neos UI (I actually created neos/neos-ui#2836 which I totally forgot about 🙈 ) and then either directly (re-)adding list styles as a core feature (list styles were actually supported in the "old" UI with CKE4) or adjust this plugin. I'm just not experienced enough with webpack and react to do this myself.

I'm happy to chat about this further and help where possible 🙂

@paxuclus paxuclus added the question Further information is requested label May 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants