t/344: Refactoring of the dropdown ecosystem

[jodator]

Suggested merge commit message (convention)

Enhancement: Introduce SplitButtonView and new dropdowns helper methods. Closes ckeditor/ckeditor5#5434. Closes ckeditor/ckeditor5#5441.

BREAKING CHANGE: Removed DropdownModel interface - use dropdown properties directly. See ckeditor/ckeditor5#5441.
BREAKING CHANGE: Removed createButtonDropdown() and ButtonDropdownView. See ckeditor/ckeditor5#5441.
BREAKING CHANGE: Removed createListDropdown() and ListDropdownView. See ckeditor/ckeditor5#5441.

Changes in other repositories:

• https://github.com/ckeditor/ckeditor5-alignment/tree/t/ckeditor5-ui/344
• https://github.com/ckeditor/ckeditor5-heading/tree/t/ckeditor5-ui/344
• https://github.com/ckeditor/ckeditor5-theme-lark/tree/t/ckeditor5-ui/344
• https://github.com/ckeditor/ckeditor5-utils/tree/t/ckeditor5-ui/344

Merge commit message for related branches:

Internal: Aligned code to the changes in the dropdowns API (see ckeditor/ckeditor5#5441).

---

Additional information

So this is initial PR with SplitButton (ckeditor/ckeditor5#5434) and Dropdowns cleanup (ckeditor/ckeditor5#5441) - mostly to open some discussion on how to close ckeditor/ckeditor5#5441 as there are some things to consider here.

To quickly see how dropdowns are created now checkout Framework Guide Stub with two examples. This file is created mostly for PR but might be expanded if needed for guides.

To see how new dropdowns are used in features checkout:

• Alignment: https://github.com/ckeditor/ckeditor5-alignment/blob/t/ckeditor5-ui/344/src/alignmentui.js#L93-L137
• Font:
  • Family: https://github.com/ckeditor/ckeditor5-font/blob/t/ckeditor5-ui/344/src/fontfamily/fontfamilyui.js#L70-L93
  • Size: https://github.com/ckeditor/ckeditor5-font/blob/t/ckeditor5-ui/344/src/fontsize/fontsizeui.js#L65-L89
• Headings: https://github.com/ckeditor/ckeditor5-heading/blob/t/ckeditor5-ui/344/src/heading.js#L102-L126
• Highlight: https://github.com/ckeditor/ckeditor5-highlight/blob/82010b1dbb5e7c60fd67cc64d6b8e787b6775990/src/highlightui.js#L153-L232

Or checkout: ckeditor5#t/ckeditor5-highlight/3 branch with proper mgit.json configuration (Sorry for branches names but that issue evolved a bit).

As suggested I've tried to create minimal set of methods. What I like here is that button appearance is controlled by model.

---

Some things that might be improved

1. method naming (as always) and maybe location inside ui package
2. All features adds own CSS class - maybe make this configurable by model / helper?

// example:
dropdownView.extendTemplate( {
	attributes: {
		class: 'ck-splitbutton-dropdown'
	}
} );

3. All features adds execute command & focus editing view afterward - helper?

// Execute current action from dropdown's split button action button.
dropdownView.on( 'execute', () => {
	// This line is similar but need command name and param:
	editor.execute( 'command', { value: model.commandValue } );
	// This line is duplicated always:
	editor.editing.view.focus();
} );

4. I had method addDefaultBehavior() which called three methods but I've removed to meke less methods. Below code is duplicated everywhere:

// Add default behavior of dropdown
closeDropdownOnBlur( dropdownView );
closeDropdownOnExecute( dropdownView );
focusDropdownContentsOnArrows( dropdownView );

5. Whether to pass toolbar/listview contents by model or by method param (I'm for the latter)
6. Toolbar - ListView dropdowns inconsistency in adding view property

// for toolbar:
dropdownView.toolbarVIew;
// for listview:
dropdownView.listView;
// maybe just:
dropdownView.contentView; // ??

---

Things that require fixes:

• The SplitButton's CSS & button inside dropdown must be fixed (awaiting theme refactor as @oleq worked on that arrow thingy).
• "execute" event is not delegated properly for SplitButton (to check at least) - see guide both "execute" are on different elements.
• docs :D

[jodator]

OK I've updated the PR with the following changes:

• instead of creating button and then dropdown there are now two functions that creates empty dropdown with given button
  • createDropdown() (maybe needs name change?)
  • createSplitButtonDropdown()
• there are no default behavior methods adding default behavior is done by above methods
• the addListViewToDropdown() and addToolbarToDropdown() now accepts dropdown and items or buttons respectively
• there is no model needed - all bindings are now on dropdownView directly (maybe should go on dropdownView.button 🤔
• I've created bindOneToMany() binding

Most changes are visible on fremework guide: fee7755...t/344#diff-1cd2b60b887d6d0735a58a4331b24806

Updated usage in features:

• Alignment: https://github.com/ckeditor/ckeditor5-alignment/blob/1f598198fde5be2853dfa76cb87eafe1c6081489/src/alignmentui.js#L86-L123
• Font Family: https://github.com/ckeditor/ckeditor5-font/blob/dd25c6e8396e5650ed98ffa9e8c838995845dd2d/src/fontfamily/fontfamilyui.js#L56-L83
• Font Size: https://github.com/ckeditor/ckeditor5-font/blob/dd25c6e8396e5650ed98ffa9e8c838995845dd2d/src/fontsize/fontsizeui.js#L50-L78
• Heading: https://github.com/ckeditor/ckeditor5-heading/blob/44627f1f9343da0dbbd96063b54ccb48ede2d084/src/heading.js#L74-L111
• Highlight: https://github.com/ckeditor/ckeditor5-highlight/blob/fdb76bc5e88cfcac9167471e9d633268338378be/src/highlightui.js#L176-L251

[Reinmar]

Some notes from the call:

• Move bindOneToMany() to ckeditor5-utils as foo.bind( 'x' ).toMany().
• Remember about default values of the dropdown view so we don't have to set them to false after creating a dropdown.
• "Not an array above" misses period. And we miss "the" a lot.
• https://github.com/ckeditor/ckeditor5-highlight/blob/d1e0d04706503bd04e507ac4c88fbf80d27ac35f/src/highlightui.js#L189 is unnecessary and it could be called simply value.

[coveralls]

Coverage remained the same at 100.0% when pulling 7c0e6c9 on t/344 into 7973f83 on master.

[Reinmar]

Do we need to have all these label, withText, etc properties on DropdownView? I assume that the button is accessible through DropdownView#button so one can set them directly there?

Although, when I look at the bindings in createDropdown() it may turn out that it won't be that easy. Anyway, curious to see the result, but I'm not sure yet whether this is the right direction.

[jodator]

@Reinmar & @oleq I've udpated this PR with changes:

• no binding from dropdown (use dropdown.buttonView instaed to control button)
• introduced new interfaces for buttons
• removed createSplitButtonDropdown() method -> the createDropdown() method now accepts a ButtonClass parameter
• updated the docs ;)

[Reinmar]

What is this? ;) Is it an interface, is it a class or is it a plain?

[jodator]

It's a copy-paste artifact.

[Reinmar]

I don't like using the word Interface in names of interfaces but sometimes it's just super hard to find a way around this. There's already Plugin and PluginInterface because I wasn't able to name both without using the Interface suffix.

[Reinmar]

Doesn't this describe the split button?

[jodator]

Yeah - it should be "fired when the button is clicked".

[Reinmar]

This is ugly. Why is the createDropdown() function responsible for that? What if I'd create another button view which implements the same interface as the dropdown button but is not an instance of DropdownButtonView?

[Reinmar]

We should delegate this logic out of here.

[Reinmar]

For the future: https://github.com/ckeditor/ckeditor5-utils/issues/221

[Reinmar]

OK, we're not going to use the word Interface by just stripping it. So there will be the Button interface and the ButtonView class. It's not great, but the actual correct naming would be ButtonViewInterface which is catastrophically long. E.g. we'd have module:ui/dropdown/button/dropdownbuttonviewinterface~DropdownButtonViewInterface. Let's go the other way around. These are only interface names so we can always change them later if we wish.

[Reinmar]

OK, I closed this PR and branches in the utils, alignment, theme-lark and heading repos. I leave the rest to you, @jodator.

Also, I reported ckeditor/ckeditor5#831 and plan to report issue about the if () in the createDropdown() function.