-
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
Navigation: Removes the header from the navigation list view in the experiment #46070
Conversation
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
Size Change: -129 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
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 agree with this from a visual standpoint. However I'm concern re: accessibility. Will it still be possible for someone using assistive tech that they are on the Menu
panel?
<Heading | ||
className="wp-block-navigation-off-canvas-editor__title" | ||
level={ 2 } | ||
> | ||
{ __( 'Menu' ) } | ||
</Heading> |
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'm not sure we should remove this from the DOM. Is it accessible?
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.
Try using Voiceover and see what is announced as you tab into the panel. If it's nothing then we need to retain a heading. If we want to lose that visually we can utilise <VisuallyHidden>
.
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 added a VisuallyHidden
header that says Menu, but when I tested with VoiceOver I didn't hear it. Not sure what's going on...
This is working as intended, my only concern is the same as Dave's |
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.
Ok I've tested this. Weirdly Voiceover doesn't announce the heading. I think that's because its not focusable.
But...that's the same issue we have with the other tabs where panel headings such as Color
are not read out either!
I think it's a wider issue. At least we've retained an accessible title.
As an aside, I think we'll need to find a better title other than "List View". It's not very descriptive of the contents of the tab. Maybe "Entity View"? That's for another PR though.
b75e993
to
bcda4c6
Compare
bcda4c6
to
ae2e961
Compare
…xperiment (WordPress#46070) * Navigation: Removes the header from the navigation list view * Add a visually hidden menu header for accessiblity
I somehow managed to miss this, but I think we need to revert this PR. As the goal of our experiment is to test the initial assumptions of the experiment, looking to see if users can find menu changing options in less proeminent controls was one of the goalposts. The original idea to move the menu selector in a dotted menu was to make a situation that is somewhat not common (multiple menus) less invasive in the UI. The heading should be removed when we can configure the inspector's top part to show the dot menu, but until then it's the best we've got. |
What?
In the Navigation List View experiment, we don't need to have a heading above the list view, and therefore hide the navigation menu selector - we can just display the selector directly.
Why?
This exposes these controls more readily, as suggested by @mtias.
How?
Removes a lot of the branching for the experiment, and makes the code a lot simpler too.
Testing Instructions
There are a few different cases to consider:
Consider what happens when the off canvas editing experiment is enabled and disabled
Consider what happens when the block inspector tabs experiment is enabled and disabled
Screenshots or screencast