Ability to configure split button in config.toolbar property #2094

engineering-this · 2018-06-15T14:29:10Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

This feature is rather straight forward. I've selected base as t/1679 so GH displays real diff for this branch.

Closes #2091

mlewand

The direction is good, and it works pretty well.

But the biggest concern now is that due to that change, now the API becomes less consistent. I'm talking about default option here.

I don't like the fact that we have more than one way to set the default action of the Split Button, that is:

set the default action name directly in Split Button configuration object
set the default action name Split Button's items, by adding default: true

It seems to me that the first option (having it directly in splitbutton definition, single prop) is better, and should be the only one to remain.

mlewand · 2018-06-17T09:31:19Z

plugins/toolbar/plugin.js

@@ -535,6 +535,12 @@

 					// Ignore items that are configured to be removed.
 					if ( !removeButtons || CKEDITOR.tools.indexOf( removeButtons, name ) == -1 ) {
+						// Option to define Split Button or other UI elements in `config.toolbar` #2091
+						if ( item.type ) {


Please simplify 2 subsequent ifs to just a single if ( cond1 && cond2 ){}.

mlewand · 2018-06-17T09:44:26Z

plugins/toolbar/plugin.js

@@ -535,6 +535,12 @@

 					// Ignore items that are configured to be removed.
 					if ( !removeButtons || CKEDITOR.tools.indexOf( removeButtons, name ) == -1 ) {
+						// Option to define Split Button or other UI elements in `config.toolbar` #2091


This is not only limited to Split Button UI element alone. Similarly you can now create regular buttons, dropdowns etc. So adjust the docs so that it does refer to known UI types.

Also when doing inline docs follow the regular format, that is:

place ticket reference in parenthesis,

end the sentence with a dot.

See the following as an example: https://github.com/ckeditor/ckeditor-dev/blob/ecfbb1e/plugins/stylescombo/plugin.js#L145

mlewand · 2018-06-17T09:50:25Z

tests/plugins/splitbutton/manual/toolbar.md

@@ -0,0 +1,11 @@
+@bender-ui: collapsed
+@bender-tags: feature, 4.10.0, 1679


Ticket in tag should refer to this particular feature issue id, rather than the whole Split Button feature.

mlewand · 2018-06-17T09:52:58Z

plugins/splitbutton/plugin.js

@@ -34,7 +43,21 @@
 		}

 		for ( i in allItems ) {
-			item = allItems[ i ];
+			itemName = item = allItems[ i ];


You're using itemName in just a single place, that is:

https://github.com/ckeditor/ckeditor-dev/blob/ecfbb1e/plugins/splitbutton/plugin.js#L56

You don't have to create a temp variable for the name to access it there, the name is still available at item.name. Use it, and remove itemName - this will reduce the complexity.

mlewand · 2018-06-17T09:56:36Z

plugins/splitbutton/plugin.js

 			previousButton,
 			defaultButton;

 		if ( face ) {
+			// Split Button can be defined with simpler definition via strings, needed by #2091


Format should be adjusted same as before. This also occurs later on in this PR, check for other occurrences.

mlewand · 2018-06-17T10:41:25Z

plugins/splitbutton/plugin.js

 			previousButton,
 			defaultButton;

 		if ( face ) {
+			// Split Button can be defined with simpler definition via strings, needed by #2091
+			if ( typeof face === 'string' ) {
+				face = editor.ui.items[ face ];


You assigning an object that you modify few lines below. That means you're modifying referenced icon.

We can not do that, you should clone it first and work on it clone instead.

mlewand · 2018-06-17T10:47:52Z

tests/plugins/splitbutton/splitbutton.js

+							items: items
+						} );
+
+						editor.ui.add( 'customclick', CKEDITOR.UI_SPLITBUTTON, {


Code registered as customclick handler is duplicated, extract it to common parts.

mlewand · 2018-06-17T10:49:26Z

tests/plugins/splitbutton/splitbutton.js

+							face: {
+								icon: 'underline',
+								click: function() {
+									counter.faceClick++;


The whole counter concept could be simplified by using sinon's stubs. Since you're using them in tests, you could just define stubs in test-scope or in context of tests object.

Then you'd want to reset it in setup/teardown function to make sure that test can not be affected by other unit tests.

Refactor of tests/plugins/splitbutton/splitbutton.js is extracted to #2098

engineering-this · 2018-06-19T12:38:43Z

I've pushed to this branch commit form #2098.

mlewand · 2018-06-20T06:24:04Z

plugins/splitbutton/plugin.js

+				face = CKEDITOR.tools.clone( editor.ui.items[ face ] );
+
+				if ( !face.icon ) {
+					face.icon = face.name || face.command;


What is the use case for this? If there is no icon in the original button it's fine to leave it without an icon.

Actually this one can be removed, but next case is required, explanations below.

mlewand · 2018-06-20T06:25:34Z

plugins/splitbutton/plugin.js

+				item = CKEDITOR.tools.clone( editor.ui.items[ item ] );
+
+				if ( !item.icon ) {
+					item.icon = item.name || item.command;


Similar case as before.

That is how button plugin works.
https://github.com/ckeditor/ckeditor-dev/blob/6c0469b530cbc767946379e00b5aedcb01e2ba5a/plugins/button/plugin.js#L262-L266

If there is no buttonDefinition.icon property, then icon will fallback to name or command. But menu plugins doesn't do that, so in order for menu item to have an icon we need to find that icon same way as button plugin does.

Example opening ~/workspace/ckeditor/ckeditor-dev/samples/index.html and executing following in console:

CKEDITOR.instances.editor.ui.get( 'Bold' );

Returned object will not have any icon property. Actually when writing this I'm thinking if maybe we should change how button works, so it always have icon property when there is icon on button. WDYT @mlewand?

As for assigning the icon property during button creation - this sounds reasonable if only we started with such approach. Now there might be 3rd party integrations that rely on this property, and rely on it being a "falsy" value. So such change could break some solutions, we don't want to break things.

This rationale sounds reasonable. In this case it make sense to revert this fallback, thanks for clarifying.

Please just drop a short note, or a link to your comment here.

Also make sure to convert the code link above to a perma link, so something like that: https://github.com/ckeditor/ckeditor-dev/blob/6c0469b530cbc767946379e00b5aedcb01e2ba5a/plugins/button/plugin.js#L262-L263

mlewand · 2018-06-20T06:46:01Z

plugins/splitbutton/plugin.js

@@ -94,12 +115,12 @@

 			editor.addFeature( properties.buttons[ item.id ] );

-			// First button as default. It might be overwritten by actual default button.
-			if ( !defaultButton && !item[ 'default' ] ) {
+			// If no default button was defined then first button will act as default.


This logic could be now simplified. Here, in the loop, you could just leave if ( definition[ 'default' ] && definition[ 'default' ] === item.name ) { check over here.

Then the second if should go outside the loop, and that would check for conditions:

whether defaultButton was not picked, with the logic above ☝️

whether definition[ 'default' ] was not defined at all

Then assign the first item as a default button (if there was at least one item).

mlewand · 2018-06-20T07:02:02Z

plugins/splitbutton/plugin.js

 				previousButton = properties.buttons[ item.id ];
 				defaultButton = properties.buttons[ item.id ];
 			} else {
-				if ( item[ 'default' ] ) {
+				if ( definition[ 'default' ] && definition[ 'default' ].toLowerCase() === item.name ) {


definition.default should be given as a button name, thus it should not be case-sensitive. We expect name Bold, Image etc.

This is case when we define split button in short forms with items as strings.

To get an item via ui.get we need to pass an uppercased name eg. Bold. But this uppercased name exists only in ui.items / ui.instances as a key.
Object returned via ui.get has property name lowercased.

Example:
~/workspace/ckeditor/ckeditor-dev/samples/index.html

var bold = CKEDITOR.instances.editor.ui.get( 'bold' ); console.log( bold ); // undefined bold = CKEDITOR.instances.editor.ui.get( 'Bold' ); console.log( bold.name ) // 'bold'

mlewand · 2018-07-16T10:39:18Z

plugins/button/plugin.js

@@ -448,9 +448,9 @@
 	 * @param {String} definition.command The command to be executed once the button is activated.
 	 * @param {String} definition.toolbar The {@link CKEDITOR.config#toolbarGroups toolbar group} into which
 	 * the button will be added. An optional index value (separated by a comma) determines the button position within the group.
-	 * @param {String} definition.style The optional inline style that will be applied on button. Custom button styles are supported sins the **4.10.0** version.
+	 * @param {String} definition.style The optional inline style that will be applied on button. Custom button styles are supported sins the **4.11.0** version.


Typo in "are supported sins the"

mlewand · 2018-07-16T11:26:15Z

plugins/toolbar/plugin.js

@@ -535,6 +535,10 @@

 					// Ignore items that are configured to be removed.
 					if ( !removeButtons || CKEDITOR.tools.indexOf( removeButtons, name ) == -1 ) {
+						// Option to define UI element in `config.toolbar` (#2091).
+						if ( item.type && item.type in editor.ui._.handlers ) {


This feature of supporting type property is nowhere to be found in the API docs. It needs to be reflected in API docs.

mlewand · 2018-07-16T11:27:44Z

tests/plugins/splitbutton/manual/splitbutton.md

@@ -1,5 +1,5 @@
 @bender-ui: collapsed
-@bender-tags: feature, 4.10.0, 1679
+@bender-tags: feature, 4.11.0, 1679


Technically this change is out of scope of this issue. It would be nice if you could extract this commit from history and move it to a proper feature branch - but it's not a big deal if it will remain here in case it's tough to do so.

mlewand · 2018-07-16T11:33:25Z

plugins/splitbutton/plugin.js

+				item = CKEDITOR.tools.clone( editor.ui.items[ item ] );
+
+				if ( !item.icon ) {
+					item.icon = item.name || item.command;


As for assigning the icon property during button creation - this sounds reasonable if only we started with such approach. Now there might be 3rd party integrations that rely on this property, and rely on it being a "falsy" value. So such change could break some solutions, we don't want to break things.

This rationale sounds reasonable. In this case it make sense to revert this fallback, thanks for clarifying.

Please just drop a short note, or a link to your comment here.

Also make sure to convert the code link above to a perma link, so something like that: https://github.com/ckeditor/ckeditor-dev/blob/6c0469b530cbc767946379e00b5aedcb01e2ba5a/plugins/button/plugin.js#L262-L263

mlewand

The code breaks if you set default with an invalid string. Such cases could happen and it should not break the editor.

For instance:

Open https://github.com/ckeditor/ckeditor-dev/blob/b2cb7cc/tests/plugins/splitbutton/manual/toolbar.html#L9-L24 and use the following snippet:

	CKEDITOR.replace( 'editor', {
		toolbar: [ [ {
			type: 'splitbutton',
			name: 'BasicStylesSplitButton',
			label: 'Basic styles',
			face: 'Bold',
			items: [ 'Bold' , 'Italic', 'Underline' ]
		} ], [ {
			type: 'splitbutton',
			name: 'JustifySplitButton',
			label: 'Justify',
			items: [ 'JustifyLeft' , 'JustifyCenter', 'JustifyRight', 'JustifyBlock' ],
			'default': 'JustifyRight123'
			} ]
		]
	} );

Of course it must be covered by a unit test.

engineering-this · 2018-07-18T11:51:48Z

Rebased onto updated #1679 branch. See comment.
Also dropped commits updating version number, as it is now corrected in feature branch.

…ned as strings

…ig.toolbar

Removed item.default property default item should be defined in definition.default

…t item

…ems defined as objects

engineering-this · 2018-11-20T12:55:08Z

Rebased, as parent branch was rebased also.

engineering-this force-pushed the t/1679 branch from 4ec0e24 to f90aa72 Compare June 15, 2018 14:31

engineering-this force-pushed the t/2091 branch from db3399b to ecfbb1e Compare June 15, 2018 14:32

mlewand self-requested a review June 17, 2018 08:22

mlewand suggested changes Jun 17, 2018

View reviewed changes

engineering-this requested a review from Comandeer June 18, 2018 13:26

engineering-this mentioned this pull request Jun 18, 2018

Refactor tests/plugins/splitbutton/splitbutton.js from t/2091 branch #2098

Closed

mlewand self-requested a review June 20, 2018 06:23

mlewand suggested changes Jun 20, 2018

View reviewed changes

mlewand self-requested a review July 16, 2018 09:45

mlewand reviewed Jul 16, 2018

View reviewed changes

mlewand suggested changes Jul 16, 2018

View reviewed changes

engineering-this force-pushed the t/1679 branch from f90aa72 to d9ec97b Compare July 18, 2018 11:47

engineering-this force-pushed the t/2091 branch from b2cb7cc to 06a32fb Compare July 18, 2018 11:49

engineering-this self-assigned this Jul 18, 2018

engineering-this force-pushed the t/2091 branch 4 times, most recently from 4f9a760 to 46f2eec Compare July 19, 2018 08:22

engineering-this requested a review from mlewand July 19, 2018 08:26

engineering-this force-pushed the t/1679 branch from c97c489 to f67f714 Compare November 20, 2018 12:03

engineering-this force-pushed the t/2091 branch from 46f2eec to 507a5ec Compare November 20, 2018 12:49

engineering-this added 7 commits November 20, 2018 13:52

Added unit test for button with custom style and button without icon

db939f6

Added new test case, fixed failing test

9325b9f

Added option to define Split Button in config.toolbar with items defi…

69d12cc

…ned as strings

Added manual test

de0e3fe

Added new editor for test with Split Button instances defined in conf…

047c40d

…ig.toolbar

If item.name is missing it will fallback to item.command

49a0c42

Removed item.default property default item should be defined in definition.default

Simplified if statement, changed comment

64ff2bc

engineering-this added 13 commits November 20, 2018 13:52

Changed Split Button definition to include new way of defining defaul…

c4eeb19

…t item

Cloning face and items to prevent modyfying original objects

198be63

Fixed comment

1474c2a

Fixed case with uppercased name not recognised as default item

d1fe55d

Refactored test

bcacf0d

Removed reduntant comma

e510478

Minor refactoring

1d407bb

Updated version tag number

da1ee31

Added docs for virtual class toolbar.item which represents toolbar it…

1251dad

…ems defined as objects

Updated docs entries

955869e

Fixed case when split button has wrong default item

ae06108

Added comment on reverted callback

e29814a

Added test case

362a192

engineering-this force-pushed the t/2091 branch from 507a5ec to 362a192 Compare November 20, 2018 12:52

engineering-this removed their assignment May 30, 2019

f1ames closed this Mar 20, 2020

f1ames added the pr:frozen ❄ The pull request on which work was suspended due to various reasons. label Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ability to configure split button in config.toolbar property #2094

Ability to configure split button in config.toolbar property #2094

engineering-this commented Jun 15, 2018

mlewand left a comment

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

mlewand Jun 17, 2018

engineering-this Jun 18, 2018 •

edited

Loading

engineering-this commented Jun 19, 2018

mlewand Jun 20, 2018

engineering-this Jun 20, 2018

mlewand Jun 20, 2018

engineering-this Jun 20, 2018 •

edited

Loading

mlewand Jul 16, 2018

mlewand Jun 20, 2018

mlewand Jun 20, 2018

engineering-this Jun 20, 2018

mlewand Jul 16, 2018

mlewand Jul 16, 2018

mlewand Jul 16, 2018

mlewand Jul 16, 2018

mlewand left a comment •

edited

Loading

engineering-this commented Jul 18, 2018

engineering-this commented Nov 20, 2018

		@@ -0,0 +1,11 @@
		@bender-ui: collapsed
		@bender-tags: feature, 4.10.0, 1679

Ability to configure split button in config.toolbar property #2094

Ability to configure split button in config.toolbar property #2094

Conversation

engineering-this commented Jun 15, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this Jun 18, 2018 • edited Loading

Choose a reason for hiding this comment

engineering-this commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engineering-this Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand left a comment • edited Loading

Choose a reason for hiding this comment

engineering-this commented Jul 18, 2018

engineering-this commented Nov 20, 2018

engineering-this Jun 18, 2018 •

edited

Loading

engineering-this Jun 20, 2018 •

edited

Loading

mlewand left a comment •

edited

Loading