i/5597: Set balloon toolbar's max-width by panr · Pull Request #536 · ckeditor/ckeditor5-ui

panr · 2020-01-14T14:16:25Z

Suggested merge commit message (convention)

Feature: The BalloonToolbar plugin should group items when its width is close to related editable's. Closes ckeditor/ckeditor5#5597. Closes ckeditor/ckeditor5#5501.

BREAKING CHANGE: The BalloonToolbar plugin groups overflowing items now by default. This can be disabled via the editor configuration by setting config.balloonToolbar.shouldNotGroupWhenFull = true.

BalloonPanelView.defaultPositions has been extended with additional positions. Please refer to the documentation to learn more.

CI and constellation

Requires:

…th and make it groupable

coveralls · 2020-01-14T14:19:45Z

Coverage remained the same at 100.0% when pulling 0395c4f on i/5597 into 334270b on master.

…esting purposes

…hen toolbar should be shown

…enFull option to the toolbar view constructor

…toolbar config

oleq

Some changes are necessary to put things right.

Also, as I commented in the twin PR, we need to update ToolbarView grouping on maxWidth change.

src/toolbar/balloon/balloontoolbar.js

src/toolbar/toolbarview.js

tests/toolbar/balloon/balloontoolbar.js

oleq · 2020-01-27T10:55:54Z

tests/toolbar/balloon/balloontoolbar.js

+
+			setData( model, '<paragraph>b[ar]</paragraph>' );
+
+			expect( document.body.contains( viewElement ) ).to.be.true;


Let's do it via utils/dom/global. Besides, why do we need this assertion in the first place? Shouldn't this test fail anyway if the editable was detached from DOM?

Why should I use utils/dom/global only here? Any other test uses /*global document... */.
And I check the assertion here just because I like to know exactly where test breaks and why if so ¯_(ツ)_/¯

If you really think that's redundant, I'll remove it ;-)

tests/toolbar/toolbarview.js

src/toolbar/balloon/balloontoolbar.js

…y work in certain scenarios

- Remove radundant helper for setting max-width - Add condintion checking if editable element is in the DOM already - Minor fix in the constructor code

- Fix test descriptions - Use global from utils and new Rect instance to get a proper width that contains padding

oleq · 2020-02-03T12:41:03Z

src/toolbar/balloon/balloontoolbar.js

+
+					// In the balloon editor toolbar's max-width should be set to the 2/3 of the editable's width.
+					// It's a safe value, because at the moment we don't re-calculate it when position of the selection changes.
+					this.toolbarView.maxWidth = toPx( new Rect( editableElement ).width * 0.66 );


I checked this ratio and I came to the conclusion that it's unsafe for very small editables, e.g.:

Unfortunately, this case is totally valid, people may enable the editor on very narrow fields.

So here's what I propose:

instead 2/3, let's go with .9. This should still be safe and it will look better in cases like this where there's plenty of space left

Let's have a sane min-width for the BalloonToolbar in CSS (so at least the heading dropdown is visible + maybe 2 or 3 buttons). As a custom CSS property so developers can override it if they want to. min-width should overrule max-width set by us in JS and this should work out great.

Can't we use the viewport width instead of editable width?

I'm not sure about the viewport thing, because in some situations, someone can have the editor's width set to, for example: 60% of the viewport and then we want to make sure, that toolbar will not expand over the editor, covering some important stuff next to it.

I agree with @panr. The UI of the editor should be restricted to the editable whenever possible. This is "our territory" in the application and we should not expand beyond that.

instead 2/3, let's go with .9. This should still be safe and it will look better in cases like this where there's plenty of space left

I .9 is too much ATM. You can have that scenario:

So if you want to set it to .9 we need to tweak anchoring behaviour as well.

We could extend default BalloonPanelView positions and add intermediate ones and then enable them in the BalloonToolbar.

And we even have an issue for that :D

Done, I added "middle-" states.

… the balloon toolbar

panr · 2020-02-06T14:19:21Z

Let's have a sane min-width for the BalloonToolbar in CSS (so at least the heading dropdown is visible + maybe 2 or 3 buttons). As a custom CSS property so developers can override it if they want to. min-width should overrule max-width set by us in JS and this should work out great.

Just added it. I set a default min-width of the balloon toolbar to 180px to show only a heading dropdown and grouped items ([...]). Heading and 2-3 buttons might be too much for super narrow editors (placed in sidebars or narrow panels).

panr · 2020-02-24T19:46:05Z

Sorry, I messed up with history merging fast forward master to this branch (stupid VSC) 😢

src/panel/balloon/balloonpanelview.js

src/toolbar/balloon/balloontoolbar.js

src/toolbar/toolbarview.js

tests/toolbar/balloon/balloontoolbar.js

…hTo for better positioning flexibility of the UI.

…tch editable width by using a mock resize observer.

…ew docs and manual tests

… well

…zeObserver class

oleq · 2020-02-26T10:11:23Z

@ckeditor/qa-team Can you guys take a look at this PR constellation? Changes potentially affect all floating balloons in the editor.

… test.

…ses.

Mgsy · 2020-02-26T13:52:40Z

We've checked it and everything looks good 👍

Fix: Set balloon toolbar's max-width to be half of the editable's wid…

bb78e2e

…th and make it groupable

panr added 11 commits January 15, 2020 15:26

Tests: Add scenario for a setting toolbar max-width

82747ef

Tests: Add scenario to test if toolbar will group items

085c255

Fix: Refactor setting max-width for toolbar as a private method for t…

ac3c31f

…esting purposes

Add maxWidth observable to ToolbarView class

69560b9

Refactor toolbar view maxWidth binding

438d844

Move setting toolbar max-width to initializer instead of running it w…

f630220

…hen toolbar should be shown

Add test for toolbar max-width binding

56d7300

Change test for setting balloon toolbar max-width

aaa73fe

Refactor access to balloon toolbar config and pass down shouldGroupWh…

fd6151a

…enFull option to the toolbar view constructor

Add test for checking if grouping items can be disabled from balloon …

d44f716

…toolbar config

Increase test timeout while waiting for ResizeObserver

08a5058

panr mentioned this pull request Jan 24, 2020

i/5597: Set inline toolbar's max-width ckeditor/ckeditor5-editor-inline#65

Merged

oleq self-requested a review January 27, 2020 09:03

oleq suggested changes Jan 27, 2020

View reviewed changes

Reinmar assigned oleq Jan 28, 2020

panr mentioned this pull request Jan 29, 2020

i/5597: Remove the note that the toolbar.shouldNotGroupWhenFull ckeditor/ckeditor5-core#211

Merged

panr added 5 commits January 29, 2020 17:07

Add _enableGroupingOnMaxWidthChange() private method

bb97620

Update ToolbarView#maxWidth observable with note describing how it ma…

b3da94f

…y work in certain scenarios

Fix ballon toolbar code

fcc0e29

- Remove radundant helper for setting max-width - Add condintion checking if editable element is in the DOM already - Minor fix in the constructor code

Remove obsolete test from ToolbarView tests

8e04fe0

Fix balloon toolbar tests

6f9eeae

- Fix test descriptions - Use global from utils and new Rect instance to get a proper width that contains padding

panr requested a review from oleq January 30, 2020 09:56

oleq suggested changes Feb 3, 2020

View reviewed changes

Add "ck-balloon-toolbar_min-width" class to set default min-width for…

a316c8d

… the balloon toolbar

panr and others added 3 commits February 21, 2020 14:03

Use ResizeObserver class instead of getResizeObserver helper

8e4882e

Make notifications context plugin.

09475dc

Export body collection to the separate file.

ffa11c3

panr added 4 commits February 24, 2020 16:59

Update ballonpanelview tests

eebfe70

Update balloontoolbar tests

0012531

Merge branch master into i/5597

e255a4c

Merge branch 'master' into i/5597

b7ddada

panr requested a review from oleq February 24, 2020 20:02

oleq suggested changes Feb 25, 2020

View reviewed changes

oleq and others added 13 commits February 25, 2020 14:33

Changed the order of default positions used by BalloonPanelView#attac…

733e30a

…hTo for better positioning flexibility of the UI.

Tests: Stabilized the BalloonToolbar that checks items grouping to ma…

d069d43

…tch editable width by using a mock resize observer.

Fix northArrowSouthMiddleWest calculations & rearrange balloonpanelvi…

a58ac73

…ew docs and manual tests

Make resizeObserver protected value

763e13c

Remove setting min-width class for balloontoolbar and related test as…

c8ea412

… well

Fix docs for ToolbarView API

24840cb

Fix manual test helpers for balloonpanelview

3d5b192

Fix typo

684123c

Remove .ck-balloon-toolbar_min-width class from theme css

c2faec9

Fix comma dangles

276e9a0

Docs: Fixed typos in BalloonPanelView.defaultPositions docs.

1c4eb28

Reverted changes to BalloonPanelViews tests made in 733e30a.

d339b70

Remove unnecessary checking for editableElement since we use new Resi…

964995c

…zeObserver class

oleq added 6 commits February 26, 2020 11:27

Tests: Improved the readability of the balloon panel view positioning…

79f7cd1

… test.

Docs and code refactoring in BalloonPanelView and BalloonToolbar clas…

56cfbe8

…ses.

Docs: Improved docs of the ToolbarView class.

3c39173

Docs: Improved the dev docs in the BalloonToolbar plugin.

8142f58

Tests: Code refactoring in BalloonPanelView tests.

85651a9

Tests: Minor naming improvement in the BalloonToolbar test.

0395c4f

oleq merged commit d36fd23 into master Feb 26, 2020

oleq deleted the i/5597 branch February 26, 2020 14:21


		setData( model, '<paragraph>b[ar]</paragraph>' );

		expect( document.body.contains( viewElement ) ).to.be.true;

Conversation

panr commented Jan 14, 2020 • edited by oleq Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Suggested merge commit message (convention)

Uh oh!

coveralls commented Jan 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oleq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

panr commented Feb 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

panr commented Feb 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oleq commented Feb 26, 2020

Uh oh!

Mgsy commented Feb 26, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

panr commented Jan 14, 2020 •

edited by oleq

Loading

coveralls commented Jan 14, 2020 •

edited

Loading

panr commented Feb 6, 2020 •

edited

Loading