I/6146 multiline toolbar #8507

pkwasnik · 2020-11-23T11:26:02Z

Suggested merge commit message (convention)

Feature (ui, toolbar): Multiline toolbar. Closes #6146.

Additional information

For example – encountered issues, assumptions you had to make, other affected tickets, etc.

…coupled document editor build.

…re used. Disable the flex wrapping only when shouldGroupWhenFull and isFloating toolbar options are enabled.

pkwasnik · 2020-11-23T11:51:12Z

Hello @ckeditor/qa-team 👋, could you take a quick look at this feature?

Mgsy · 2020-11-25T10:14:39Z

We've tested it and works fine 👍

oleq

The PR looks good. I added some comments mostly concerning the docs.

@godai78, can you double-check this?

@wwalc We're waiting for your feedback regarding

Let's go with declarative multi-line toolbar for the document editor build. I will recheck what features could land there taking this occasion that we will have more space there.

from #6146 (comment). If you want to do this asynchronously, let us know to not block this PR 🙏

oleq · 2020-11-26T13:07:14Z

docs/builds/guides/integration/configuration.md

+    }
+```
+
+ * **`items`** &ndash; An array of toolbar item names. The components (buttons, dropdowns, etc.) which can be used as toolbar items are defined in `editor.ui.componentFactory` and can be listed using the following snippet: `Array.from( editor.ui.componentFactory.names() )`


I know this is a straight copy from https://ckeditor5.github.io/docs/nightly/ckeditor5/latest/api/module_core_editor_editorconfig-EditorConfig.html#member-toolbar but mentioning componentFactory (which is a pretty low-level API) here in the guide for people who see the editor for the first time may not be the best idea.

OTOH I don't know how we could avoid mentioning it. We don't have any guide for the component factory but it is used in the "Creating a simple plugin" guide so maybe at least link there?

Maybe listing all items somewhere in the guide docs and linking there instead of mentioning about componentFactory?

This would be nice if we had a dedicated "toolbar guide" that was called for somewhere in the issues. List items, configuration and options. Maybe I should take it on next iteration.

oleq · 2020-11-26T13:07:55Z

docs/builds/guides/integration/configuration.md

+ Useful when a page with which the editor is being integrated has some other sticky or fixed elements
+ (e.g. the top menu). Thanks to setting the toolbar offset the toolbar will not be positioned underneath or above the page's UI.
+
+ * **`shouldNotGroupWhenFull`** &ndash; When set `true`, the toolbar will stop grouping items and let them wrap to the next line when there is not enough space to display them in a single row.


We should mention here that grouping is enabled by default.

Ok. By the way, I am considering if it will be more clear if the name will be groupWhenFull? (ofc. enabled by default).

docs/builds/guides/integration/configuration.md

oleq · 2020-11-26T13:13:23Z

docs/builds/guides/integration/configuration.md

+There are also two ways of arranging buttons in multiple lines. Both require the extended format.
+
+1. Set `shouldNotGroupWhenFull` to true, so items will wrap automatically.
+2. Follow 1st point and set the breaking point explicitly using `'-'` separator:


I would not use the word "separator" here. It can be confused with the items group separator (`|`).

I would also rephrase "Follow 1st point..." cc @godai78 .

Change separator into "divider"?

Or stay with

and set the breaking point explicitly using '-'.

docs/builds/guides/integration/configuration.md

oleq · 2020-11-26T13:43:53Z

packages/ckeditor5-ui/src/toolbar/toolbarview.js

+					 *
+					 * @error line-separator-used-when-button-grouping-enabled
+					 */
+					logWarning( 'line-separator-used-when-button-grouping-enabled' );


Maybe let's log config along with this error. There could be multiple editors with different toolbar configs on the same page and for this error to be useful we should give developers some context.

oleq · 2020-11-26T13:52:28Z

packages/ckeditor5-ui/src/toolbar/toolbarview.js

@@ -884,6 +911,13 @@ class DynamicGrouping {
 * @member {Boolean} module:ui/toolbar/toolbarview~ToolbarOptions#shouldGroupWhenFull
 */

+/**
+ * To allow declarative toolbar breaks for floating toolbars it should be set to `true`.


This is not exactly what this option is for. This should be more like:

This option should be enabled for toolbars in absolutely positioned containers without width restrictions to to enable automatic {@link module:ui/toolbar/toolbarview~ToolbarView#items} grouping.

When this option is set to `true`, items will stop wrapping to the next line and together with {@link module:ui/toolbar/toolbarview~ToolbarOptions#shouldGroupWhenFull} this will allow grouping them when there is not enough space in a single row.

+ this options should be mentioned in module:ui/toolbar/toolbarview~ToolbarOptions#shouldGroupWhenFull.

packages/ckeditor5-ui/tests/toolbar/toolbarview.js

oleq · 2020-11-26T13:54:54Z

packages/ckeditor5-ui/tests/toolbar/toolbarview.js

@@ -118,6 +119,17 @@ describe( 'ToolbarView', () => {
 			expect( view.itemsView.element.classList.contains( 'ck-toolbar__items' ) ).to.true;
 		} );

+		it( 'should include ck-toolbar_floating class if there are shouldGroupWhenFull and isFloating options set to true', () => {
+			const viewWithOptions = new ToolbarView( locale, {
+				shouldGroupWhenFull: true,


What if shouldGroupWhenFull is false? Would this work?

I've added more tests about it

packages/ckeditor5-ui/tests/toolbar/toolbarview.js

Co-authored-by: Aleksander Nowodzinski <[email protected]>

…or5 into i/6146-multiline-toolbar

docs/builds/guides/integration/configuration.md

…or5 into i/6146-multiline-toolbar

oleq and others added 6 commits November 17, 2020 13:33

Allowed multi-line ToolbarView configuration via "/" in the config.

64382b2

Added styles for multi-line toolbars.

2050712

Added more features and re-arrenged the toolbar for 2 lines in the de…

700fc02

…coupled document editor build.

Implemented warning when grouping is enabled and declarative brakes a…

91d01dc

…re used. Disable the flex wrapping only when shouldGroupWhenFull and isFloating toolbar options are enabled.

Added ToolbarLineBreakView test.

299edbe

Updated docs related to multiline toolbar.

8d57f6c

CKEditorBot added the pr:review-needed label Nov 23, 2020

pkwasnik changed the title ~~I/6146 multiline toolbar~~ WIP: I/6146 multiline toolbar Nov 23, 2020

pkwasnik changed the title ~~WIP: I/6146 multiline toolbar~~ [WIP] I/6146 multiline toolbar Nov 23, 2020

pkwasnik changed the title ~~[WIP] I/6146 multiline toolbar~~ WIP I/6146 multiline toolbar Nov 23, 2020

Changing new line separator to '-';

6aff552

pkwasnik force-pushed the i/6146-multiline-toolbar branch from 4a77277 to 6aff552 Compare November 23, 2020 21:20

pkwasnik changed the title ~~WIP I/6146 multiline toolbar~~ I/6146 multiline toolbar Nov 23, 2020

Updated toolbar docs.

395d7e8

Merge branch 'master' into i/6146-multiline-toolbar

e7498a1

oleq self-assigned this Nov 26, 2020

Merge branch 'master' into i/6146-multiline-toolbar

f8f763f

oleq self-requested a review November 26, 2020 12:55

CKEditorBot removed the pr:review-needed label Nov 26, 2020

oleq requested changes Nov 26, 2020

View reviewed changes

godai78 and others added 8 commits November 26, 2020 15:05

Update docs/builds/guides/migrate.md

97dbde6

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Update packages/ckeditor5-core/src/editor/editorconfig.jsdoc

d822ca3

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Update packages/ckeditor5-core/src/editor/editorconfig.jsdoc

5380f24

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Update packages/ckeditor5-ui/tests/toolbar/toolbarview.js

29cad9d

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Update packages/ckeditor5-ui/tests/toolbar/toolbarview.js

7e77290

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Update docs/builds/guides/integration/configuration.md

c2d2a8f

Co-authored-by: Aleksander Nowodzinski <[email protected]>

Docs: minor fixes.

ad8070e

Docs: minor fixes.

1f6e4f1

godai78 and others added 3 commits November 26, 2020 15:24

Docs: naming conventions.

8d2fa0f

Merge branch 'i/6146-multiline-toolbar' of github.com:ckeditor/ckedit…

6c6bf27

…or5 into i/6146-multiline-toolbar

Review corrections.

fc56ba2

pkwasnik requested review from oleq and godai78 November 27, 2020 16:40

godai78 reviewed Nov 30, 2020

View reviewed changes

docs/builds/guides/integration/configuration.md Outdated Show resolved Hide resolved

Docs: minor fixes. [skip ci]

ec1504c

godai78 reviewed Nov 30, 2020

View reviewed changes

docs/builds/guides/integration/configuration.md Outdated Show resolved Hide resolved

pkwasnik and others added 3 commits November 30, 2020 12:44

Docs update.

27df32f

Merge branch 'i/6146-multiline-toolbar' of github.com:ckeditor/ckedit…

81ca018

…or5 into i/6146-multiline-toolbar

Docs: minor fix.

7f614e6

godai78 approved these changes Nov 30, 2020

View reviewed changes

oleq added 4 commits November 30, 2020 15:56

Merge branch 'master' into i/6146-multiline-toolbar

3296b5c

Docs.

6e5a8c9

Tests: Fixed a failing ToolbarView test.

4154bc0

Docs.

729b9ba

oleq merged commit 70157ae into master Dec 1, 2020

oleq deleted the i/6146-multiline-toolbar branch December 1, 2020 10:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I/6146 multiline toolbar #8507

I/6146 multiline toolbar #8507

pkwasnik commented Nov 23, 2020 •

edited

Loading

pkwasnik commented Nov 23, 2020

Mgsy commented Nov 25, 2020

oleq left a comment

oleq Nov 26, 2020

pkwasnik Nov 27, 2020

godai78 Nov 30, 2020

oleq Nov 26, 2020

pkwasnik Nov 27, 2020

oleq Nov 26, 2020

oleq Nov 26, 2020

godai78 Nov 26, 2020

oleq Nov 26, 2020

pkwasnik Nov 30, 2020

oleq Nov 26, 2020

pkwasnik Nov 30, 2020

oleq Nov 26, 2020

pkwasnik Nov 30, 2020

oleq Nov 26, 2020

pkwasnik Nov 30, 2020

I/6146 multiline toolbar #8507

I/6146 multiline toolbar #8507

Conversation

pkwasnik commented Nov 23, 2020 • edited Loading

Suggested merge commit message (convention)

Additional information

pkwasnik commented Nov 23, 2020

Mgsy commented Nov 25, 2020

oleq 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

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

pkwasnik commented Nov 23, 2020 •

edited

Loading