-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add a vertical toolbar for zoom out mode #60123
Conversation
Size Change: +476 B (+0.03%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
Flaky tests detected in b1b5d7a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9578571343
|
I'm going to focus first on the other items in #60124 before continuing this exploration |
437dc3c
to
5633e87
Compare
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
I think it would be nice if the toolbar stuck to the top of the screen. |
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.
Aside from some small tweaks I suggested, this looks good from the components side 👍
e4c4303
to
259597b
Compare
I addressed the review of the component's changes. Do we think this is a good starting point for the toolbar on zoom out mode? /cc @richtabor |
like a fixed toolbar? I'm not sure if I understand what this would entail |
d3a08b5
to
e2a6d60
Compare
|
||
const { removeBlock } = useDispatch( blockEditorStore ); | ||
|
||
const classNames = clsx( 'zoom-out-toolbar', { |
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 wonder if vertical-toolbar
is a better name as this could be used in other context.
@@ -14,6 +13,10 @@ | |||
margin: 0; | |||
} | |||
|
|||
.components-accessible-toolbar:not(.zoom-out-toolbar) & { | |||
background: $white; |
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 think it would be better to just remove this altogether, but there is a risk that it's used for something...
@@ -20,6 +20,12 @@ | |||
|
|||
.components-accessible-toolbar, | |||
.components-toolbar { | |||
|
|||
&[aria-orientation="vertical"] { |
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 moved this down so that it also targets components-toolbar
as well as components-accessible-toolbar
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.
This is looking pretty good to me. My main concerns are:
- Should we rename zoom out toolbar to vertical toolbar or something more generic?
- Is there too much duplication in the
ZoomOutPopover
andZoomOutToolbar
components - would it be better to compose the base components?
I think the fact that is vertical is just an extra. The CSS for the component will be available to any other future vertical toolbars, the rest of the changes are specific to the needs of zoom out (mainly the contents of the toolbar we need it to have in this specific instance of it) |
b1b5d7a
to
81f4326
Compare
Co-authored-by: Lena Morita <lena@jaguchi.com>
This reverts commit 59e5bdb.
b690f15
to
e1f50a0
Compare
* position toolbar popover to the left and change colors on zoomed-out mode * Make zoom out toolbar component * Create zoom out toolbar * Cleanup linting errors * Add orientation to toolbar component * css for vertical toolbars * Add similar classes to existing toolbar * Use correct placement for zoom out toolbar popover * remove cs that shouldn't be needed * Remove icon and block toolbar vertical css and apply just to base component * made toolbar unstyled and fixed stray background * Update Changelog * removed transparent background from component css * Update packages/components/src/toolbar/toolbar/types.ts Co-authored-by: Lena Morita <lena@jaguchi.com> * document new prop for the toolbar component * remove white background * increase margin width * Also include the non-accessible toolbar in the vertical CSS rules * Move styles to the zoom out toolbar CSS * revert * avoid shifting of the toolbar to the right side * fix focus outline * moved vertical styles to toolbar component * missing display flex * removed orientation from NavigableToolbar props * Revert "removed orientation from NavigableToolbar props" This reverts commit 59e5bdb. * Use ToolbarButton instead of Button for Shuffle * added classes to avoid using component selectors --------- Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Ben Dwyer <ben@scruffian.com>
What?
Closes #59737
Ariakit supports vertical toolbars so we implemented a new variant to the main
Toolbar
component and applied it to a new toolbar specifically for the zoom-out mode.Why?
Zoom-out mode needs a toolbar that doesn't cover the content we are inserting. We needed to leverage the Ariakit toolbar for accessibility, so the black pseudo-toolbar that we were using wasn't enough.
How?
By implementing a toolbar that extends
NavigableToolbar
and adding a new orientation prop that ariakit usesTesting Instructions
Open the inserter and open a pattern category
Click on a section, the toolbar should be visible on the side. All buttons should work as usual
Testing Instructions for Keyboard
When you focus on a section, move focus to the toolbar and use up and down arrows to navigate between the toolbar buttons
Screenshots or screencast