Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

t/ckeditor5/416d: Implemented automatic items grouping in the ToolbarView component #523

Merged
merged 45 commits into from
Oct 15, 2019

Conversation

oleq
Copy link
Member

@oleq oleq commented Sep 16, 2019

Suggested merge commit message (convention)

Feature: Implemented automatic items grouping in the ToolbarView component. Closes ckeditor/ckeditor5#416.

BREAKING CHANGE: The internal structure of the component has changed. Toolbar items are no longer direct descendants of the toolbar in DOM, which may affect some integrations (mainly CSS selectors if adjustments were made to the styles).


Additional information

Requires:

Constellation for testing and development.

@oleq
Copy link
Member Author

oleq commented Sep 16, 2019

Some concerns:

  • We may need throttling on top of the ResizeObserver,
  • ...?

@oleq
Copy link
Member Author

oleq commented Sep 16, 2019

@Mgsy Could you check the constellation and play with the new feature a little bit? There's a manual test ready but also all ClassicEditors and DecoupledEditors in docs should have it enabled.

@jodator
Copy link
Contributor

jodator commented Oct 14, 2019

The same bug with Chrome rendering problems :( I'm not reporting this as I feel that we track that already.

Selection_025

@jodator
Copy link
Contributor

jodator commented Oct 14, 2019

This is a bug or a feature? 😃
Peek 2019-10-14 18-51

@jodator
Copy link
Contributor

jodator commented Oct 14, 2019

Might be a follow-up: I cannot type after executing a command.
Steps:

  1. Use alt+f10 to focus toolbar
  2. Navigate to overflow menu
  3. execute some command

Actual (focus is lost)
Peek 2019-10-14 19-03

Expected (focus is somewhere - like in normal toolbar)
Peek 2019-10-14 19-04

@jodator
Copy link
Contributor

jodator commented Oct 14, 2019

ps. the very, very, very long toolbar is lost on the other side ;)
Peek 2019-10-14 19-05

Copy link
Contributor

@jodator jodator left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor things detected. I couldn't find anything in particular that should needed to be changed. The code looks OK.

I also find some minor issues with the behavior but nothing blocking though (maybe except that execute on overflowed toolbar).

src/toolbar/toolbarview.js Outdated Show resolved Hide resolved
src/toolbar/toolbarview.js Outdated Show resolved Hide resolved
src/toolbar/toolbarview.js Outdated Show resolved Hide resolved
src/toolbar/toolbarview.js Show resolved Hide resolved
src/toolbar/toolbarview.js Outdated Show resolved Hide resolved
src/toolbar/toolbarview.js Outdated Show resolved Hide resolved
theme/components/toolbar/toolbar.css Outdated Show resolved Hide resolved
@oleq
Copy link
Member Author

oleq commented Oct 15, 2019

The same bug with Chrome rendering problems :( I'm not reporting this as I feel that we track that already.

Yes.

This is a bug or a feature? 😃

Never planned this but this is definitely a feature 😛

Might be a follow-up: I cannot type after executing a command.

It's a general issue. I added a comment explaining the scope of the problem.

ps. the very, very, very long toolbar is lost on the other side ;)

Yup. Created a follow-up.

@jodator jodator merged commit 46911bf into master Oct 15, 2019
@jodator jodator deleted the t/ckeditor5/416d branch October 15, 2019 10:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Responsive toolbar
4 participants