-
Notifications
You must be signed in to change notification settings - Fork 40
Add unwrapElement method to UpcastWriter #1772
Conversation
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 method should be similar to the writer.unwrap()
from model writer.
src/view/upcastwriter.js
Outdated
this.remove( element ); | ||
this.insertChild( index, element.getChildren(), parent ); | ||
|
||
return true; |
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 method should not return true/false - we don't need that.
Check the model's writer.unwrap()
how we handle this:
ckeditor5-engine/src/model/writer.js
Lines 851 to 861 in ad9159c
if ( element.parent === null ) { | |
/** | |
* Trying to unwrap an element which has no parent. | |
* | |
* @error writer-unwrap-element-no-parent | |
*/ | |
throw new CKEditorError( 'writer-unwrap-element-no-parent: Trying to unwrap an element which has no parent.', this ); | |
} | |
this.move( Range._createIn( element ), this.createPositionAfter( element ) ); | |
this.remove( element ); |
After some clarification from @msamsel I see that some methods of upcast writer return true/false in similar scenarios but TBH I don't see why. So I'm for not returning anything but adding a |
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.
Now it's OK :)
* @param {module:engine/view/element~Element} element Element which will be unwrapped | ||
* @returns {Booolean} Whether element was successfully unwrapped. | ||
*/ | ||
unwrapElement( 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.
Just for my curiosity. Why not just unwrap
? Other methods from this class (like remove
, replace
, rename
) don't have Element
suffix.
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've followed by this explanation:
ckeditor/ckeditor5-paste-from-office#72 (comment)
Suggested merge commit message (convention)
Other: Add unwrapElement method to UpcastWriter.
Additional information
PR extracted from ckeditor/ckeditor5#2499`