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

Widget marker conversion handles only classes #7359

Closed
scofalik opened this issue Jun 2, 2020 · 3 comments
Closed

Widget marker conversion handles only classes #7359

scofalik opened this issue Jun 2, 2020 · 3 comments
Labels
package:widget resolution:expired This issue was closed due to lack of feedback. status:stale type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

scofalik commented Jun 2, 2020

📝 Provide a description of the improvement

When a view element is widgetized, it gets its own handling of markers that are created on that widget. All widgets get that by default. Thanks to that we have borders on images or tables when we add comments or other markers to it.

This is the part of the code that is responsible for that: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-widget/src/utils.js#L109-L114

As you can see, this mechanism handles only classes. This is different than handling marker conversion for texts: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/conversion/downcasthelpers.js#L924

For texts, we create a view attribute element from the descriptor and we wrap the text with it. This is very simple:

export function createViewElementFromHighlightDescriptor( writer, descriptor ) {

As you can see, attributes are handled as well (priority does not make sense for non-attribute view elements).

We add data-comment="" attribute to for comments markers (in editing downcast). I just noticed that it is not set for widgets. We didn't get any issue reports, though.

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. package:widget labels Jun 2, 2020
@scofalik
Copy link
Contributor Author

scofalik commented Jun 2, 2020

I also believe that setHighlightHandling() should have third and fourth parameters be optional. Or I'd introduce setDefaultHighlightHandling() function that would do that. I believe that in a lot of use cases we will use just simple class+attributes conversion.

@Reinmar Reinmar added this to the next milestone Jul 20, 2020
@Reinmar Reinmar added the type:bug This issue reports a buggy (incorrect) behavior. label Jul 20, 2020
@pomek pomek modified the milestones: next, nice-to-have Nov 10, 2020
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added the resolution:expired This issue was closed due to lack of feedback. label Nov 13, 2023
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:widget resolution:expired This issue was closed due to lack of feedback. status:stale type:bug This issue reports a buggy (incorrect) behavior. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

No branches or pull requests

4 participants