Navigation Block List View: Improve the accessible Name of the Tree Grid inside the component. #47031
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
scruffian
changed the title
|
Size Change: +82 B (0%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
getdave
reviewed
Jan 10, 2023
getdave
reviewed
Jan 10, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
|
Flaky tests detected in ff21a8f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4073329420
|
talldan
reviewed
Jan 11, 2023
Comment on lines
54
to
67
| const label = navigationMenu | ||
| ? sprintf( | ||
| /* translators: %s: The name of a menu. */ | ||
| __( 'Navigation menu: %s' ), | ||
| navigationMenu.title | ||
| ) | ||
| : __( 'You have not yet created any menus. Displaying a list of your Pages' ); |
There was a problem hiding this comment.
The piece that's missing is that this doesn't tell the user what kind of UI they're going to encounter. It might be worth keeping something closer to 'Block navigation structure' or 'List view' as the label in that respect. Possibly 'Navigation block list view'.
My feeling is that the context about the current state of the menu could be added as an aria description, as this is secondary information.
Would be good to get @alexstine's opinion on this.
There was a problem hiding this comment.
Yes. I do wonder whether mirroring what you did in #47047 would be a good option. Use an description to announce what the thing does.
This definitely requires accessibility feedback from an expert in this field.
See #44291 (review) for some context. TLDR, it may be best to keep the duplication. It could be worth adding a comment to the code to explain it. |
|
I think |
alexstine
reviewed
Jan 21, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
scruffian
force-pushed
the
fix/47022
branch
2 times, most recently
from
9887783
to
83c184e
Compare
|
Ok I've updated this to use If you're happy with this then I'll also apply it to the List View component. Sidenote - |
|
That sounds good to me. Thanks. |
getdave
reviewed
Jan 31, 2023
| ? sprintf( | ||
| /* translators: %s: The name of a menu. */ | ||
| __( 'Structure for navigation menu: %s' ), | ||
| navigationMenu.title |
There was a problem hiding this comment.
We should handle a situation where this title could be undefined. Just in case. I think I saw @talldan catch a similar bug on the Page List recently.
getdave
reviewed
Jan 31, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
scruffian
commented
Feb 1, 2023
packages/block-library/src/navigation/edit/menu-inspector-controls.js
Outdated
Show resolved
Hide resolved
Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Dave Smith <getdavemail@gmail.com> Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com> switch to using a description rather than another label Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Dave Smith <getdavemail@gmail.com>
getdave
approved these changes
Feb 2, 2023
Mamaduka
added
the
Backport to Gutenberg RC
|
I just cherry-picked this PR to the release/15.1 branch to get it included in the next release: 767b8dd |
Mamaduka
pushed a commit
that referenced
this pull request
Feb 3, 2023
…rid inside the component. (#47031) * Improve accessible Name of the Navigation Offcanvas List View Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Dave Smith <getdavemail@gmail.com> Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com> switch to using a description rather than another label Update packages/block-library/src/navigation/edit/menu-inspector-controls.js Co-authored-by: Dave Smith <getdavemail@gmail.com> * Apply suggestions from code review * disable linting * fix linter --------- Co-authored-by: Ben Dwyer <ben@escruffian.com> Co-authored-by: Dave Smith <getdavemail@gmail.com>
Mamaduka
removed
the
Backport to Gutenberg RC
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 makes it possible to modify the name of the OffCanvasEditor component, so that it can be customized to the context it's being used in.
Why?
The previous label doesn't make sense in the Navigation List View context.
How?
Adds a prop to the OffCanvasEditor component. The List View component should also get this prop if this is the right way to achieve this.
Testing Instructions for Keyboard
Fixes #47022