Create MediaFormView for each dropdown instance separately. #98

tomalec · 2020-04-21T14:35:18Z

Suggested merge commit message (convention)

Fix: Create MediaFormView for each dropdown instance separately. Closes ckeditor/ckeditor5#6333

MINOR BREAKING CHANGE: mediaembedui~MediaEmbedUI#form was removed from the API

Additional information

Previously it was trying to attach the same MediaFormView instance form for all dropdowns instances, then attach actions multiple times.

BTW, I was thinking about refactoring a bit _setUpDropdown and _setUpForm, to have the same order of arguments, but decided to keep the changes to the minimum.

…ditor/ckeditor5#6333

coveralls · 2020-04-21T14:38:02Z

Pull Request Test Coverage Report for Build 431

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 425:	0.0%
Covered Lines:	309
Relevant Lines:	309

💛 - Coveralls

Reinmar · 2020-04-21T20:38:13Z

tests/mediaembedui.js

@@ -53,6 +53,12 @@ describe( 'MediaEmbedUI', () => {
 		expect( dropdown ).to.be.instanceOf( DropdownView );
 	} );

+	it( 'should allow creating two instances', () => {


To make this test a bit more bulletproof, it'd be good to ensure that two instances are actually created. What if someone changes the beforeEach to not create this item (I can imagine this line being moved to particular it()s?

So, I'd simply call this .create() twice in this test.

It'd also be good to verify that it's not the same instance returned twice :D

Reinmar · 2020-04-21T20:39:02Z

src/mediaembedui.js

 		// Setup `imageUpload` button.
 		editor.ui.componentFactory.add( 'mediaEmbed', locale => {
 			const dropdown = createDropdown( locale );

-			this._setUpDropdown( dropdown, this.form, command, editor );
-			this._setUpForm( this.form, dropdown, command );
+			// Prepare custom view for dropdown's panel.


I think that while taken the bug this line may seem in need of being commented, this is actually completely logical code and the test will be enough.

Reinmar · 2020-04-21T20:39:55Z

BTW, I was thinking about refactoring a bit _setUpDropdown and _setUpForm, to have the same order of arguments, but decided to keep the changes to the minimum.

Haha :D My OCD suffers here as well. I think it's fine if it lands in another commit.

Reinmar · 2020-04-21T20:42:41Z

src/mediaembedui.js

@@ -41,19 +41,15 @@ export default class MediaEmbedUI extends Plugin {
 		const command = editor.commands.get( 'mediaEmbed' );
 		const registry = editor.plugins.get( MediaEmbedEditing ).registry;

-		/**


It's a breaking change. We need to mention this in the merge commit message (can be "MINOR BREAKING CHANGE"). What I'd also check is why the heck was this exposed on the plugin and whether someone doesn't use it somewhere (you can ping @scofalik to check CF's code for this). In general, it's odd that it was exposed here, so it's worth doublechecking.

Updated the merge commit message.

Checked ckeditor5 project for [\."']form\b - nothing found, any ideas where else should I check?

Reinmar

Some minor issues as commented.

…ent from each other. As suggested at #98 (review)

…ove unnecessary comment. Agreed at https://github.com/ckeditor/ckeditor5-media-embed/pull/98/files/4952dc3bc743a462d542b81f7eb5c696a398c34c#r412475345 #98 (comment)

tomalec · 2020-04-22T10:29:48Z

All comments addressed, except on unclear thing
#98 (review)

still waiting for @scofalik
it seems it was intentionally added at 04d138f#diff-773e307f93ae07fbe6347534966073e7R44. @oleq, I see why static get pluginName was added: Various plugins are missing names making it hard to access their properties when editor is running ckeditor5#1336, but why this.form was exposed? Is it used somewhere else?

oleq · 2020-04-22T11:09:01Z

but why this.form was exposed? Is it used somewhere else?

Probably because people (including me) pointed out in some issues that there's no way to access the form and its descendants from the editor instance object. The form hosts several components and integrators may want to access them and adjust things (labels, placeholders, etc.).

As a rule of thumb, there should always be some way to access any piece of UI by a chain of public properties starting with an editor instance (or editor.ui). Otherwise, we're creating a monolith.

As for why I didn't document or test that change, well... that's another topic :P Sorry for the confusion. I don't remember why I decided to do that in an unrelated PR.

tomalec · 2020-04-22T12:16:36Z

If we are about to expose a form as property on MediaEmbedUI instance, we'd rather have a single instance. However, I'd achieve it by changing the behavior of the media embed button, to behave more like a link element, and pop-up a form where the caret is, rather than in the dropdown.

That would be more like UX change, and I'm not sure whether it's in the scope of this issue, and if it requires any approval from somebody? (@Reinmar ?)

oleq · 2020-04-22T12:56:54Z

We definitely shouldn't change the UX of the feature in this PR.

Solutions:

Unless CF is using the reference to the media form, I'm OK with making it private again and scoped to the component returned by the factory.
We could also leave the public MediaEmbedUI#form reference. There's a possibility of sharing a single MediaFormView between multiple dropdowns and it's pretty simple. Because it's unlikely both will be open at the same time, we can inject the form to the currently open dropdown panel as it opens:

diff --git a/src/mediaembedui.js b/src/mediaembedui.js
index ec46f40..50ca212 100644
--- a/src/mediaembedui.js
+++ b/src/mediaembedui.js
@@ -48,12 +48,20 @@ export default class MediaEmbedUI extends Plugin {
                    */
                   this.form = new MediaFormView( getFormValidators( editor.t, registry ), editor.locale );

+                  this.form.urlInputView.bind( 'value' ).to( command, 'value' );
+
+                  // Form elements should be read-only when corresponding commands are disabled.
+                  this.form.urlInputView.bind( 'isReadOnly' ).to( command, 'isEnabled', value => !value );
+
+                  this.form.saveButtonView.bind( 'isEnabled' ).to( command );
+
                   // Setup `imageUpload` button.
                   editor.ui.componentFactory.add( 'mediaEmbed', locale => {
                            const dropdown = createDropdown( locale );

                            this._setUpDropdown( dropdown, this.form, command, editor );
-                           this._setUpForm( this.form, dropdown, command );
+
+                           this.form.delegate( 'submit', 'cancel' ).to( dropdown );

                            return dropdown;
                   } );
@@ -65,7 +73,14 @@ export default class MediaEmbedUI extends Plugin {
                   const button = dropdown.buttonView;

                   dropdown.bind( 'isEnabled' ).to( command );
-                  dropdown.panelView.children.add( form );
+
+                  dropdown.on( 'change:isOpen', () => {
+                           if ( dropdown.isOpen ) {
+                                    dropdown.panelView.children.add( form );
+                           } else {
+                                    dropdown.panelView.children.remove( form );
+                           }
+                  } );

                   button.set( {
                            label: t( 'Insert media' ),
@@ -102,15 +117,6 @@ export default class MediaEmbedUI extends Plugin {
                            dropdown.isOpen = false;
                   }
          }
-
-         _setUpForm( form, dropdown, command ) {
-                  form.delegate( 'submit', 'cancel' ).to( dropdown );
-                  form.urlInputView.bind( 'value' ).to( command, 'value' );
-
-                  // Form elements should be read-only when corresponding commands are disabled.
-                  form.urlInputView.bind( 'isReadOnly' ).to( command, 'isEnabled', value => !value );
-                  form.saveButtonView.bind( 'isEnabled' ).to( command );
-         }
 }

tomalec · 2020-04-22T13:12:55Z

Because it's unlikely both will be open at the same

That is a thing I wasn't sure about. I could imagine a theme, a11y mode, or just a UX
change where opened toolbars are left open until explicitly closed. If you are fine with 1., personally I'd go that way due to YAGNI.

Reinmar · 2020-04-22T21:44:55Z

Can't we expose the form property on the button itself? I wouldn't feel safe with the form being reused as this may lead to strange situations. We're talking about corner cases here, but what if someone wants to have one of these forms look a bit different because it's displayed in the block toolbar (so perhaps should be more narrow) and the other form in the normal toolbar? Etc.

If the form is shown by the button, then the button should expose it. I guess that this is what we'd do if we weren't biased by seeing this bug first and were writing this for the first time :D We just started exploring more complex areas because of what we already saw.

Reinmar · 2020-04-22T21:51:32Z

personally I'd go that way due to YAGNI.

That'd be fine for me too. However, while we don't see an application for accessing the form, it often happens that it appears at the moment when we simply have other things to do. And then someone needs to wait at least a month until we release a new version. Of course, there's always a way to fork this particular module or the entire package, so making decisions about such things always troubles me :D A trivial change can save someone a lot of work. But will anyone actually need it?

tomalec · 2020-04-23T09:25:26Z

Can't we expose the form property on the button itself? ...

I think we should rather expose it on dropdown, as that's what we add to the toolbar, and that's where we add the form.

Let me know if I get correctly what you mean by exposing, and making future applications easier.

	const mediaForm = new MediaFormView( /*...*/ );
	dropdown.panelView.children.add( mediaForm );

	// DevX sugar to avoid dropdown.panelView.children.get(0).
	dropdown.form = mediaForm;

	dropdown.panelView.children.get(0) === dropdown.form // true

But then how to document it? What is the JSDOC handle for dropdown - element instance returned by editor.ui.componentFactory.add ?

Reinmar · 2020-04-23T10:39:08Z

I think we should rather expose it on dropdown, as that's what we add to the toolbar, and that's where we add the form.

👍 Sorry, that's what I meant.

But then how to document it?

Oh, right. I didn't think about this. Yup, we'd need to subclass DropdownView just to document this propert 🤦 For me that makes all this not worth the effort (the ROI drops).

So, as you proposed, let's drop this property completely. After all, it can be accessed via dropdown.panelView.children.get(0), that I didn't even know.

Reinmar · 2020-04-23T10:40:41Z

Waiting for confirmation from @scofalik that we can safely remove this property.

Reinmar · 2020-04-29T12:54:46Z

Waiting for confirmation from @scofalik that we can safely remove this property.

Szymon said we're fine.

Reinmar · 2020-04-30T10:33:23Z

We're waiting with the merge for the monorepo migration.

…ent from each other. As suggested at ckeditor/ckeditor5-media-embed#98 (review)

…ove unnecessary comment. Agreed at https://github.com/ckeditor/ckeditor5-media-embed/pull/98/files/4952dc3bc743a462d542b81f7eb5c696a398c34c#r412475345 ckeditor/ckeditor5-media-embed#98 (comment)

tomalec · 2020-05-05T15:02:49Z

Closing in favor of ckeditor/ckeditor5#6736

Create MediaFormView for each dropdown instance separately. Fixes cke…

4952dc3

…ditor/ckeditor5#6333

tomalec mentioned this pull request Apr 21, 2020

Editor crashes after inserting two mediaEmbed items to the toolbar ckeditor/ckeditor5#6333

Closed

Reinmar reviewed Apr 21, 2020

View reviewed changes

Reinmar suggested changes Apr 21, 2020

View reviewed changes

tomalec added 2 commits April 22, 2020 11:24

Make sure two instances of MediaEmbed dropdown are created and differ…

aba7523

…ent from each other. As suggested at #98 (review)

Unify MediaEmbedUI#_setUpDropdown and _setUpForm arguments order, rem…

4b09b74

…ove unnecessary comment. Agreed at https://github.com/ckeditor/ckeditor5-media-embed/pull/98/files/4952dc3bc743a462d542b81f7eb5c696a398c34c#r412475345 #98 (comment)

Fix the grammar in the expectiation for two mediaEmbeds.

f8dfef7

Reinmar approved these changes Apr 23, 2020

View reviewed changes

tomalec added a commit to ckeditor/ckeditor5 that referenced this pull request May 4, 2020

Make sure two instances of MediaEmbed dropdown are created and differ…

9dd2156

…ent from each other. As suggested at ckeditor/ckeditor5-media-embed#98 (review)

tomalec mentioned this pull request May 4, 2020

I/6333: Create MediaFormView for each dropdown instance separately ckeditor/ckeditor5#6736

Merged

tomalec closed this May 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create MediaFormView for each dropdown instance separately. #98

Create MediaFormView for each dropdown instance separately. #98

tomalec commented Apr 21, 2020 •

edited

Loading

coveralls commented Apr 21, 2020 •

edited

Loading

Reinmar Apr 21, 2020

Reinmar Apr 21, 2020

Reinmar commented Apr 21, 2020

Reinmar Apr 21, 2020

tomalec Apr 22, 2020

Reinmar left a comment

tomalec commented Apr 22, 2020

oleq commented Apr 22, 2020

tomalec commented Apr 22, 2020

oleq commented Apr 22, 2020 •

edited

Loading

tomalec commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020

tomalec commented Apr 23, 2020

Reinmar commented Apr 23, 2020

Reinmar commented Apr 23, 2020

Reinmar commented Apr 29, 2020

Reinmar commented Apr 30, 2020

tomalec commented May 5, 2020

Create MediaFormView for each dropdown instance separately. #98

Create MediaFormView for each dropdown instance separately. #98

Conversation

tomalec commented Apr 21, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Apr 21, 2020 • edited Loading

Pull Request Test Coverage Report for Build 431

💛 - Coveralls

Reinmar Apr 21, 2020

Choose a reason for hiding this comment

Reinmar Apr 21, 2020

Choose a reason for hiding this comment

Reinmar commented Apr 21, 2020

Reinmar Apr 21, 2020

Choose a reason for hiding this comment

tomalec Apr 22, 2020

Choose a reason for hiding this comment

Reinmar left a comment

Choose a reason for hiding this comment

tomalec commented Apr 22, 2020

oleq commented Apr 22, 2020

tomalec commented Apr 22, 2020

oleq commented Apr 22, 2020 • edited Loading

tomalec commented Apr 22, 2020

Reinmar commented Apr 22, 2020

Reinmar commented Apr 22, 2020

tomalec commented Apr 23, 2020

Reinmar commented Apr 23, 2020

Reinmar commented Apr 23, 2020

Reinmar commented Apr 29, 2020

Reinmar commented Apr 30, 2020

tomalec commented May 5, 2020

tomalec commented Apr 21, 2020 •

edited

Loading

coveralls commented Apr 21, 2020 •

edited

Loading

oleq commented Apr 22, 2020 •

edited

Loading