-
Notifications
You must be signed in to change notification settings - Fork 40
T/1226 Refactored how markers removal is converted from the model to the view #1347
Conversation
Related PR ckeditor/ckeditor5-link#178 |
src/view/writer.js
Outdated
* @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements | ||
* with the same `id` or `null` if given element had no `id`. | ||
*/ | ||
getAllClonedElements( element ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The getter should not be a writer method. It should be a method of an element. You should not require creating view.change
block to call getter, because this block causes rendering.
src/conversion/mapper.js
Outdated
|
||
for ( const element of boundElements ) { | ||
if ( element.is( 'attributeElement' ) ) { | ||
for ( const clone of element.getCustomProperty( 'clonedElements' ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't getAllClonedElements
be used here?
src/view/writer.js
Outdated
} | ||
|
||
group.add( element ); | ||
this.setCustomProperty( 'clonedElements', group, element ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would use a protected property instread of a custom property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only make sure it is cloned well ;)
…are broken during wrapping. Introduced `view.Writer#getAllBrokenSiblings`. Changed: Changed how `view.Writer` wraps and unwraps elements, now it bases on `view.AttributeElement#id` too. Removed: Removed `HighlightAttributeElement`.
…roller` to `conversion.DowncastDispatcher`. Changed: Use markers binding provided by `conversion.Mapper` in `conversion.downcast-converters`.
…s groups and how `conversion.Mapper` uses this information.
src/view/attributeelement.js
Outdated
*/ | ||
get priority() { | ||
return this._priority; | ||
} | ||
|
||
/** | ||
* Element identifier. If set, it is used by {@link module:engine/view/element~Element#isSimilar}, | ||
* and then two elements are considered similar if, and only if they have the same `_id`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
id
src/view/attributeelement.js
Outdated
* Throws {@link module:utils/ckeditorerror~CKEditorError attribute-element-get-elements-with-same-id-no-id} | ||
* if this element has no `id`. | ||
* | ||
* @returns {Set.<module:engine/view/attributeelement~AttributeElement>|null} Set containing all the attribute elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will never return null
.
Suggested merge commit message (convention)
Other: Refactored how markers removal is converted from the model to the view. Closes ckeditor/ckeditor5#4233.