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

t/ckeditor5-media-embed/1: Made the view stringify util output the content of the UI element #1506

Merged
merged 5 commits into from
Aug 23, 2018

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 20, 2018

Suggested merge commit message (convention)

Other: Made the view stringify util output the content of the UIElement (see ckeditor/ckeditor5#2727).


A piece of ckeditor/ckeditor5-media-embed#2 constellation.

@coveralls
Copy link

coveralls commented Aug 20, 2018

Coverage Status

Coverage increased (+0.0008%) to 98.617% when pulling 7801f21 on t/ckeditor5-media-embed/1 into ef5960e on master.

@Reinmar
Copy link
Member

Reinmar commented Aug 21, 2018

I'm against this change, exactly because of what you had to do in ckeditor5-table:

image

Want to check the content of a UI element? Check it (by accessing it and checking what its render() returned), but don't affect the entire stringify() method which then becomes far less useful for many other cases.

Alternatively, we could make this configurable in stringify(). But I've got problems believing in innerHTML's stability across browsers, so I've got mixed feelings whether it's safe at all.

@oleq
Copy link
Member Author

oleq commented Aug 22, 2018

I went for the config option which made the table PR obsolete.

@Reinmar Reinmar merged commit 49cd795 into master Aug 23, 2018
@Reinmar Reinmar deleted the t/ckeditor5-media-embed/1 branch August 23, 2018 11:26
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.

3 participants