Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Declarative nested toolbars #12506

Merged
merged 17 commits into from
Sep 27, 2022
Merged

Declarative nested toolbars #12506

merged 17 commits into from
Sep 27, 2022

Conversation

oleq
Copy link
Member

@oleq oleq commented Sep 23, 2022

Suggested merge commit message (convention)

Feature (ui): Allowed grouping toolbar items into drop-downs by using a declarative config. Closes #12490.

BREAKING CHANGE (basic-styles): The bold icon has been moved to the ckeditor5-core package.
BREAKING CHANGE (paragraph): The paragraph icon has been moved to the ckeditor5-core package.


Additional information

TODOs:

  • Are we OK that the bold and paragraph icons moved to the ckeditor5-core package (e.g. while italic and heading1 didn't)?
  • Docs proofreading.
  • Do we want a default icon? (three vertical dots is a solid candidate)

@oleq oleq marked this pull request as ready for review September 26, 2022 14:18
@oleq oleq changed the title [WIP] Nested toolbars Nested toolbars Sep 26, 2022
@oleq oleq changed the title Nested toolbars Declarative nested toolbars Sep 26, 2022
@mlewand
Copy link
Contributor

mlewand commented Sep 27, 2022

@oleq regarding moving the icons, why core package? Wouldn't UI package be more fitting?

Regarding the default icon, where do you see it kicking in? Would that it's added as an opt-out (so that integrator has to pass icon: false to remove the icon)? Or would it be present if integrator didn't provide neither the icon nor withText property?

@mlewand mlewand requested a review from Dumluregn September 27, 2022 08:52
@arkflpc arkflpc self-requested a review September 27, 2022 08:56
@mlewand mlewand removed the request for review from Dumluregn September 27, 2022 09:00
@oleq
Copy link
Member Author

oleq commented Sep 27, 2022

@oleq regarding moving the icons, why core package? Wouldn't UI package be more fitting?

Because we're already collecting icons there. Alignment icons are there, object alignment icons too. And blockquote. All icons that could be (or already are) re-used by other features.

I don't say this is the best place, though. Honestly I don't know why we have icons scattered across the project. They don't make sense without ckeditor5-ui and the IconView class anyway. Maybe because sb. else could have their custom UI but use our icons? I don't know (remember), really. I'd rather keep all icons in the project in the ckeditor5-ui package, but this would be a huge change. It probably overlaps with #12496 too.

Regarding the default icon, where do you see it kicking in? Would that it's added as an opt-out (so that integrator has to pass icon: false to remove the icon)? Or would it be present if integrator didn't provide neither the icon nor withText property?

The latter. But this is not critical I guess. There are warnings logged if you fail to provide either an icon or withText: true, anyway.

oleq and others added 2 commits September 27, 2022 11:41
@godai78
Copy link
Contributor

godai78 commented Sep 27, 2022

I made some changes, as we use "drop-down" (hyphenated) when it's an adjective, and here we mostly had nouns.

…e to configure a drop-down without an icon and a visible text label.
@arkflpc arkflpc merged commit a0f92cb into master Sep 27, 2022
@arkflpc arkflpc deleted the ck/12490-nested-toolbars branch September 27, 2022 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Declarative nested toolbars
4 participants