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 command helpers to a better place #2847

Closed
Reinmar opened this issue Aug 22, 2016 · 9 comments
Closed

Move command helpers to a better place #2847

Reinmar opened this issue Aug 22, 2016 · 9 comments
Assignees
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@Reinmar
Copy link
Member

Reinmar commented Aug 22, 2016

See ckeditor/ckeditor5-core#10 (comment).

E.g. I noticed that none of these functions have anything to do with the core. They only know about the engine, so it seems that they can be moved there. Question – where precisely?

@Reinmar
Copy link
Member Author

Reinmar commented Jun 13, 2017

We talked with @pjasiun that those helpers would make perfect sense in the Schema.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 15, 2017

I've been also thinking what to do with ToggleAttributeCommand. It's the only command which we added to the core but only the basic-styles package is using it.

In CKE4 the style system was used heavily by a wide range of features, but that won't be the case in CKE5. Instead of building a centralised super system for dealing with block/object/inline styles, we have an abstract model and simpler utils spread around the engine (mostly).

Hence, ToggleAttributeCommand makes no sense here. I'd move it to basic-styles. If one will want to create similar command to any of our basic-styles, they should depend on basic-styles, which is fine because those commands will be thematically related anyway.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 15, 2017

BTW, besides basic styles there's also LinkCommand. In CKE4 they were all based on the style system but, of course, there was a problem with the style system's flexibility (cause link is a bit different than typical styles) so we had this lovely line:

https://github.com/ckeditor/ckeditor-dev/blob/621199c174b8df22306ca8c55742a676d8db3fe0/plugins/link/dialogs/link.js#L36

style.type = CKEDITOR.STYLE_INLINE; // need to override... dunno why.

I love the fact that this line is 9 years old :D

@Reinmar Reinmar self-assigned this Jun 16, 2017
@Reinmar
Copy link
Member Author

Reinmar commented Jun 16, 2017

Related ticket in the engine: https://github.com/ckeditor/ckeditor5-engine/issues/969.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 19, 2017

Another ticket – now basic-styles: https://github.com/ckeditor/ckeditor5-basic-styles/issues/47.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 19, 2017

And one more in the link package: https://github.com/ckeditor/ckeditor5-link/issues/131.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 19, 2017

And I can also see that getSchemaValidRanges() is used in ckeditor5-autoformat, but it shouldn't be used there. I'll try to fix https://github.com/ckeditor/ckeditor5-autoformat/issues/7 and the dependency will be gone.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 20, 2017

I had to give up with https://github.com/ckeditor/ckeditor5-autoformat/issues/7 for now, so I'll just remove the dependency on the core in https://github.com/ckeditor/ckeditor5-autoformat/issues/31.

@Reinmar
Copy link
Member Author

Reinmar commented Jun 20, 2017

All done, so we can remove these helpers from this package.

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-core Oct 9, 2019
@mlewand mlewand added this to the iteration 11 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. package:core labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:core type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

2 participants