Migrating StyleBook to use updated Composite implementation
#55344
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
andrewhayward
added
[Type] Task
andrewhayward
requested review from
noisysocks,
ciampo,
chad1008,
andrewserong and
brookewp
6 tasks
|
Size Change: +2.27 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
chad1008
approved these changes
Oct 13, 2023
There was a problem hiding this comment.
Thanks for the ping! This code change is looking good. I noticed one tiny difference in testing via keyboard between trunk and this PR:
In trunk if you use the home / end keys, then the first / last items scroll entirely into view as the home / end keys take the user all the way to the beginning or end of the scrollable area:
2023-10-16.12.02.16.mp4
With this PR, it seems that it only scrolls enough for the item to be visible, not all the way to the very end of the area, so examples can be slightly cut off:
2023-10-16.12.01.19.mp4
Would it be worth tweaking some styling with scroll-padding-top and/or scroll-padding-bottom (if that'll play nicely with Composite, that is) to ensure items are always fully visible in that case?
Good catch. Added some some scroll margins, seems to be working a bit better now. |
ciampo
reviewed
Nov 17, 2023
|
There's a small issue in the media tab, where audio and video blocks sort of remain in the tab order.
This is in Filed an issue with them – maybe it'll get resolved at some point! Works fine in Firefox/Safari. |
|
Flaky tests detected in fea8b96. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6983067267
|
ciampo
reviewed
Nov 17, 2023
| className={ className } | ||
| aria-label={ label } | ||
| role="grid" |
There was a problem hiding this comment.
What's the rationale for changing the role to grid? Does it make sense even if every row always contains only one cell?
There was a problem hiding this comment.
The grid pattern can be used to group a set of interactive elements, such as links, buttons, or checkboxes. Since only one element in the entire grid is included in the tab sequence, grouping with a grid can dramatically reduce the number of tab stops on a page. 1
In theory it should make it clearer that the buttons (as rendered by Example below) should be navigated with arrow keys rather than tab. Most screen readers will announce the number of rows too, which means users get told how many buttons are in the collection
Footnotes
There was a problem hiding this comment.
Not a blocker for this PR, but — have you considered using CompositeRow (and potentially CompositeGroup) ?
There was a problem hiding this comment.
I did, but I didn't want to move this too far away from the original implementation.
ciampo
approved these changes
Nov 22, 2023
There was a problem hiding this comment.
LGTM 🚀
Not sure if we should wait for @andrewserong to take a final look too.
There was a problem hiding this comment.
Thanks for the ping! And apologies for missing the other updates last week, thanks for the follow-up.
I'm still seeing the outline of the active item being cut off when navigating via cursor keys (but especially home and end keys). I think we might be able to resolve it by using the hard-coded value of 32px for the scroll top / bottom, to match the value that overrides padding on the body element. Just left a comment with a couple screengrabs.
Other than that, though, this is looking good to me, too!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
This PR updates
StyleBookin@wordpress/edit-siteto use the updatedCompositeimplementation from #54225, and adds additional semantic context around the example blocks.Why?
In #54225, an updated implementation of
Compositewas added to@wordpress/components. As per #55224, all consumers ofCompositeneed to migrate from the old version to the new version.How?
__unstableCompositeimports from@wordpress/componentsComposite*exports from@wordpress/componentsExamples/ExamplewithinStyleBookto use updatedCompositecomponentsCompositeto make UX clearer for assistive technology users.Testing Instructions
Navigate to the Style Book overview, by selecting "Styles" from the "Design" menu, then opening the Style Book. The style book frame should still render as one actionable button. Clicking this should take you to the main story book view.
The style book can also be accessed from the the styles sidebar header in the site editor.
Each item within the Style Book should present as an actionable button, which should open the relevant section in the styles sidebar.
Testing Instructions for Keyboard
With the main style book view open, the set of block examples should present as buttons within a grid. These should be navigable by tabbing to the grid, and using the arrow keys to move up and down.