Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Add unwrapElement method to UpcastWriter #1772

Merged
merged 2 commits into from
Aug 2, 2019
Merged

Conversation

msamsel
Copy link
Contributor

@msamsel msamsel commented Aug 2, 2019

Suggested merge commit message (convention)

Other: Add unwrapElement method to UpcastWriter.


Additional information

PR extracted from ckeditor/ckeditor5#2499`

@coveralls
Copy link

coveralls commented Aug 2, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 61cf87f on t/paste-from-office/69 into 04858be on master.

@jodator jodator self-requested a review August 2, 2019 11:31
Copy link
Contributor

@jodator jodator left a 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.

this.remove( element );
this.insertChild( index, element.getChildren(), parent );

return true;
Copy link
Contributor

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:

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 );

@jodator
Copy link
Contributor

jodator commented Aug 2, 2019

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 if( element.parent ) check to not perform unwrap on detached elements.

Copy link
Contributor

@jodator jodator left a 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 :)

@jodator jodator merged commit 9e97196 into master Aug 2, 2019
@jodator jodator deleted the t/paste-from-office/69 branch August 2, 2019 12:53
* @param {module:engine/view/element~Element} element Element which will be unwrapped
* @returns {Booolean} Whether element was successfully unwrapped.
*/
unwrapElement( element ) {
Copy link
Contributor

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.

Copy link
Contributor Author

@msamsel msamsel Aug 2, 2019

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)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants