t/344: Refactoring of the dropdown ecosystem #361

jodator · 2018-01-24T18:22:14Z

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:

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:

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

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

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

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

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

Whether to pass toolbar/listview contents by model or by method param (I'm for the latter)
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 · 2018-01-29T11:14:41Z

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:

Reinmar · 2018-02-01T09:31:25Z

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 · 2018-02-02T11:41:01Z

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

…down.

Reinmar · 2018-02-05T16:31:17Z

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 · 2018-02-06T09:53:11Z

@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 · 2018-02-06T14:33:26Z

src/button/buttoninterface.jsdoc

+ *
+ *		document.body.append( view.element );
+ *
+ * @extends module:ui/view~View


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

It's a copy-paste artifact.

Reinmar · 2018-02-06T14:36:21Z

src/button/buttoninterface.jsdoc

+/**
+ * The button interface.
+ *
+ * @interface module:ui/button/buttoninterface~ButtonInterface


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 · 2018-02-06T14:37:27Z

src/dropdown/button/dropdownbuttoninterface.jsdoc

+ */
+
+/**
+ * Fired when the arrow view is clicked. It won't be fired when the button {@link #isEnabled} is `false`.


Doesn't this describe the split button?

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

Reinmar · 2018-02-06T14:42:29Z

src/dropdown/utils.js

+
+	buttonView.bind( 'isEnabled' ).to( dropdownView );
+
+	if ( buttonView instanceof DropdownButtonView ) {


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?

We should delegate this logic out of here.

Reinmar · 2018-02-06T14:48:08Z

src/dropdown/button/splitbuttonview.js

+	render() {
+		super.render();
+
+		this.children.add( this.actionView );


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

Reinmar · 2018-02-08T13:38:06Z

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.

… it.

Internal: Aligned dropdowns usage to changed in the UI. See ckeditor/ckeditor5-ui#361.

Reinmar · 2018-02-08T14:28:17Z

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.

jodator requested review from Reinmar and oleq January 24, 2018 18:22

jodator force-pushed the t/344 branch from 66783c9 to 1058875 Compare February 2, 2018 11:34

jodator force-pushed the t/344 branch from f507baf to b868994 Compare February 2, 2018 12:09

jodator mentioned this pull request Feb 2, 2018

T/4: Initial Font plugin implementation. ckeditor/ckeditor5-font#5

Merged

jodator added 22 commits February 2, 2018 15:05

Other: Initial SplitButton implementation.

cdbc350

Other: Update SplitButtonDropdown keystrokes behavior.

7de6eb4

Other: Make createSplitButtonDropdown minimal.

3e62922

Other: Change how split button is bound to model.

537fbe6

Other: Move some SplitButtonDropdown functionality to SplitButtonDrop…

f4fe2e8

…down.

Other: Update SplitButton CSS.

7804863

Other: Fix SplitButton CSS.

575b2a9

Change: Horizontal toolbar should have nowrap set.

488d050

Changed: Make split button in toolbar look like other buttons.

4a291b9

Added: Style toolbar separator in button dropdown.

08fa387

Fix: Split button's main button outline is not displayed correctly.

2c96b24

Other: SplitButton dropdown improvements.

dbc7a70

Remove splitbuttondropdownview.js and use 'select' button event instead.

0394490

Changed: Stub new dropdown creator methods.

e1cc15c

Changed: Extract common dropdown tasks to helper methods.

102e5c7

Changed: Temporary disable 'isOn' binding in button.

ba2a0ee

Changed: Rename parts of SplitButton buttons to action/select.

79ec879

Fix: typos.

63642cf

Changed: Remove crateButtonDropdown() and createListDropdown() helpers.

b70ffca

Tests: add tests for dropdown util: enableModelIfOneIsEnabled().

80c7258

Changed: Remove createDropdown() method.

5cf51a5

Changed: Remove toolbar class binding from dropdown.

9236088

jodator added 7 commits February 5, 2018 18:49

Changed: Remove dropdown.buttonView bindings to dropdown.

6a08edc

Docs: Move dropdowns framework guide to architecture/ui-library guide.

33783eb

Docs: Introduce ButtonInterface and DropdownButtonInterface.

a0308c3

Docs: Update dropdown buttons documentation.

1413c2e

Docs: Update dropdown documentation.

41e1bbe

Docs: Add missing docs for DropdownPanelView.

2b0291d

Changed: Rename DropdownButtonInterface#select event to #open.

8344775

Reinmar reviewed Feb 6, 2018

View reviewed changes

oleq changed the title ~~T/344~~ t/344: Refactoring of the dropdown ecosystem Feb 7, 2018

Reinmar added 3 commits February 7, 2018 16:42

Merge branch 'master' into t/344

faa800b

Improved ButtonInterface and how other classes implement it.

110f4f8

Improved docs.

a908e03

jodator and others added 2 commits February 8, 2018 14:42

Tests: Update dropdown manual tests.

07925bb

Avoid using the word Interface in interface names by simply stripping…

7c0e6c9

… it.

Reinmar mentioned this pull request Feb 8, 2018

Dropdown button's arrow is misaligned ckeditor/ckeditor5#831

Closed

Reinmar merged commit 0f26ca8 into master Feb 8, 2018

Reinmar deleted the t/344 branch February 8, 2018 14:23

Reinmar added a commit to ckeditor/ckeditor5-alignment that referenced this pull request Feb 8, 2018

Merge branch t/ckeditor5-ui/344

c4fd9b8

Internal: Aligned dropdowns usage to changed in the UI. See ckeditor/ckeditor5-ui#361.

Reinmar added a commit to ckeditor/ckeditor5-heading that referenced this pull request Feb 8, 2018

Merge branch t/ckeditor5-ui/344

4108770

Internal: Aligned dropdowns usage to changed in the UI. See ckeditor/ckeditor5-ui#361.

Reinmar added a commit to ckeditor/ckeditor5-theme-lark that referenced this pull request Feb 8, 2018

Merge branch t/ckeditor5-ui/344

7999134

Internal: Aligned dropdowns usage to changed in the UI. See ckeditor/ckeditor5-ui#361.

jodator mentioned this pull request Oct 9, 2019

Manual test are not updated to the newest dropdown API. ckeditor/ckeditor5#3391

Closed

Reinmar mentioned this pull request Oct 9, 2019

Introduce Observable#bind()#toMany() ckeditor/ckeditor5#4994

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

t/344: Refactoring of the dropdown ecosystem #361

t/344: Refactoring of the dropdown ecosystem #361

jodator commented Jan 24, 2018 •

edited by pomek

Loading

jodator commented Jan 29, 2018

Reinmar commented Feb 1, 2018

coveralls commented Feb 2, 2018 •

edited

Loading

Reinmar commented Feb 5, 2018

jodator commented Feb 6, 2018

Reinmar Feb 6, 2018

jodator Feb 6, 2018

Reinmar Feb 6, 2018

Reinmar Feb 6, 2018 •

edited

Loading

jodator Feb 6, 2018

Reinmar Feb 6, 2018

Reinmar Feb 6, 2018

Reinmar Feb 6, 2018

Reinmar commented Feb 8, 2018

Reinmar commented Feb 8, 2018


		buttonView.bind( 'isEnabled' ).to( dropdownView );

		if ( buttonView instanceof DropdownButtonView ) {

t/344: Refactoring of the dropdown ecosystem #361

t/344: Refactoring of the dropdown ecosystem #361

Conversation

jodator commented Jan 24, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

jodator commented Jan 29, 2018

Reinmar commented Feb 1, 2018

coveralls commented Feb 2, 2018 • edited Loading

Reinmar commented Feb 5, 2018

jodator commented Feb 6, 2018

Reinmar Feb 6, 2018

Choose a reason for hiding this comment

jodator Feb 6, 2018

Choose a reason for hiding this comment

Reinmar Feb 6, 2018

Choose a reason for hiding this comment

Reinmar Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

jodator Feb 6, 2018

Choose a reason for hiding this comment

Reinmar Feb 6, 2018

Choose a reason for hiding this comment

Reinmar Feb 6, 2018

Choose a reason for hiding this comment

Reinmar Feb 6, 2018

Choose a reason for hiding this comment

Reinmar commented Feb 8, 2018

Reinmar commented Feb 8, 2018

jodator commented Jan 24, 2018 •

edited by pomek

Loading

coveralls commented Feb 2, 2018 •

edited

Loading

Reinmar Feb 6, 2018 •

edited

Loading