-
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
Updates the behavior of the top toolbar fixed setting #49634
Conversation
Size Change: +1.54 kB (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Flaky tests detected in 694f03d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4731389090
|
This comment was marked as resolved.
This comment was marked as resolved.
Perhaps as a follow-up we could try exploring a more graceful expand/collapse with animation. Would make it clearer for visual-based users to understand context of the controls/swapping the toolbars. |
One bit I'm not sure about, but would appreciate context, is if you collapse the toolbar, its always collapsed. Is that the expected behavior? CleanShot.2023-04-06.at.10.25.08.mp4 |
I don't know but I think it's better if the toggle sticks. |
Please do not move the fixed toolbar. Vertical screen estate can be scarce, even on my 1366×643 window, but this option is worth the 49 pixels so I know exactly where the block toolbar is at all times. And I would continue to use the document-related buttons without needing to toggle that toolbar open and then open either the block inserter or outline view. I know the keyboard shortcut for Undo, but adding another button to activate before showing the undo and redo buttons (which also have arrow icons) is not helpful for anyone who needs them. |
@sabernhardt I totally agree the collapsing is not ideal, but is is in my opinion a good trade off:
It looks like a worth it trade off. It's not only about the 49 pixels in height (out of 643, which are even less for the canvas) - but even this number on such screens is pretty big - in your case it's ~50 pixels out of ~420 vertical available, 11% of screen area won back. That is quite significant at low screen resolutions. But beyond this it's a gain in functionality which, yes, adds a complication, but it makes a setting less broken. The block editors are not desktop applications, and even less document processors. They live inside other chrome and need new models of interaction adjusted to the nature of their primary interface which is the block. What do you think? |
The vertical separators being smaller seems more difficult than hiding the toolbar in a dropdown 😂 (not seriously but seriously the way they are done do not allow for much wiggle room in terms of shrinking them - we are using the same toolbar). The fact that we're positioning the toolbar fixed it cannot be fluid. I can only respond to breakpoints. |
d6ab100
to
e4c3f2b
Compare
@jasmussen @jameskoster although I love this visually, to me all these problems with responsiveness, text labels, hiding and showing so many affordances, point at this having some hidden UX problem that instead of addressing we force this UX to fit :) |
Can you elaborate? Although maybe not the most elegant, the mockup I shared in that thread above that hides instead of covers at a particular breakpoint seems like it should work, no? I agree there are numerous ux problems to fix around text labels and responsiveness in general. But I don't think they are unique to the top toolbar option, and are worth fixing separately. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
packages/block-editor/src/components/block-tools/block-contextual-toolbar.js
Outdated
Show resolved
Hide resolved
…mobile desktop situations
1b83a38
to
5356b0a
Compare
margin-bottom: $grid-unit + $grid-unit-05; | ||
background-color: $gray-300; | ||
position: absolute; | ||
left: 44px; |
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 probably the sketchiest value of them all, maybe it needs a comment? Is it icon width + icon padding?
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 you did a great job with the CSS here
42e521e
to
5356b0a
Compare
… so that the tests pass
Thanks for all the work here! Let's see how it fares :) |
I'm late to the party since this has already been merged, but after testing this new implementation, I ran into some usability issues and it looks like a significant regression. The most noticeable and easy to test regression is when trying to undo multiple changes. Would it be possible to append the block toolbar instead of replacing the top-level one? 🤔 This constant back-and-forth is not a good experience a11y-wise, I only did a quick 5-minute test, I didn't have the time to do a full review, but it was apparent that this completely broke keyboard navigation, it's not at all easy to do simple tasks 😞 |
@aristath the undo problem that you found is a bug: undoing should not expand the collapsed block toolbar. I'll see why that is.
There are screen estate constraints that prevent this.
My main concern with this chance was to not break any a11y expectation and this weird implementation which absolutely positions the block toolbar is the only one that managed to keep a11y expectations intact. The bug that you found is not an a11y bug, it's an UX bug, a poor experience. It's also surprising, not sure what resets the state of the block toolbar. |
Thank you @draganescu for working on this PR and addressing the feedback, it's greatly appreciated!
Happy to help reviewing that PR once it's ready :) |
Is there any issue open for this? Curious what's being done to follow up here and close the loop. This does seem like a bug. Happy to report an issue if no one else does. @aristath @draganescu |
There are other options that can be evaluated to work with the space constraints. On a general note, the accessibility team has always been reporting that an UI that continuously changes making controls continuously appear and disappear is a problem. Nonetheless, this change goes in that direction. While I do understand the good intent, in my experience this kind of 'smart' UIs may seem good at first, but they rarely work for users. I doube this new implementation is any better than what we have now. A11y-wise there are more issues with this implementation at the point that I wouldn't know where to start. Some quick testing with the keyboard surfaces only some of the most evident ones. I'd encourage everyone to test this new UI with the keyboard first to get an idea of the main issues so that we can continue the conversation with some more context. On top of keyboard navigation, there are other issues related to the |
What?
Closes #40450
Updates the behavior of the top toolbar fixed setting.
Why?
Because verical screen estate is scarce and.
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
top-toolbar-goodkeyboard.mp4
Known issues