-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Add accessible Toolbar #18534
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a very quick pass through PR and left some feedback with minor notes. I hope to find some time early next week to retest the revised toolbar. As far as I can tell, this PR no longer changes the behavior of the block editor itself, it only enables a new toolbar behind the experimental flag. Awesome 👍
Travis reports some failing e2e tests but they are probably coming from master
after new blocks were added.
I need to retest it again after it was extracted to this PR and confirm that the bundle size change is still at 4 kB after GZip. Otherwise, I don't have any more code related comments. |
It works great in my testing. I tested with Chrome on macOS and Safari on iPadOS with the external keyboard. The very last step is to confirm the impact on the bundle size. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before:
./build/components/index.js 589 KiB 11 [emitted] [big] components
gutenberg % gzip -c build/components/index.js | wc -c
165507
After:
./build/components/index.js 605 KiB 11 [emitted] [big] components
gutenberg % gzip -c build/components/index.js | wc -c
170005
Result
~16 kB before gzip
~4.4 kB after simulated gzip
which is very close to what @diegohaz shared in #17875 (comment).
Let's get this PR in, it doesn't change anything in the production code so I think it's safe to proceed. @diegohaz, can you open a parent issue to track all the remaining tasks? There is one small item to consider there. How about we move There is also |
Description
This PR is a simpler version of #17875. It adds an
__experimentalAccessibilityLabel
prop to theToolbar
component in@wordpress/components
. If this prop is passed in, then it will render an accessible toolbar. Otherwise, it'll fallback to the current toolbar on master (which is not actually a toolbar) that has been renamed toToolbarGroup
.This
Toolbar
only acceptsToolbarButton
as toolbar items.ToolbarButton
is currently an enhancedIconButton
, but it should be improved in a follow-up PR.With this, projects using
@wordpress/components
could start experimenting it for simple toolbars.Story
This PR
Improve
ToolbarButton
so that it serves more use cases than just being anIconButton
.At this point, the Gutenberg header toolbar (without the fixed block toolbar) could use it.
Refactor
SlotFill
so we won't need to force fills to re-render whenfillProps
change.At this point, block toolbars could start using it.
Convert the fixed toolbar buttons (block switcher, more tools etc.) to
ToolbarButton
.At this point, custom blocks could start using it, as long as they use
ToolbarButton
insideBlockControls
orBlockFormatControls
.Convert core block toolbars.
We could convert one per PR (for example, start with the Paragraph toolbar).
Let it be for a while.
If it proves to be useful and works out, deprecate the old usage and remove the
__experimental
prefix. Otherwise, revert it altogether, which would break the code for people using__experimentalAccessibilityLabel
explicitly, but custom blocks would seamlessly rollback to the old usage.Related: #15331 #3383 #16514 #17875
How has this been tested?
npm run storybook:dev
Toolbar
storiesScreenshots
Types of changes
New feature
Checklist: