-
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
Accessibility improvement on #29530 issue #29534
Conversation
removed not optimized function
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.
Hi @Quintis1212, thanks so much for jumping on this issue!
packages/edit-site/src/components/navigation-sidebar/navigation-toggle/index.js
Outdated
Show resolved
Hide resolved
packages/edit-site/src/components/navigation-sidebar/navigation-toggle/index.js
Outdated
Show resolved
Hide resolved
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 looks good and tests well for me.
Thanks for your help here @Quintis1212 !
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import ContentNavigation from './content-navigation'; | ||
import TemplatesNavigation from './templates-navigation'; | ||
import { useSelect } from '@wordpress/data'; | ||
import { useSelect, useDispatch } from '@wordpress/data'; |
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 know you didn't add this here, and wont block the PR on it. But just noticing this import should fall under the "WordPress dependencies" group of imports above.
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.
@Addison-Stavlo thank you for help !) I updated the dependencies group )
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.
Awesome, thanks!
Will merge this for you, but it looks like Static Analysis needs to pass first (rerunning it, will investigate more if there are any issues). |
Im wondering why the static analysis keeps erroring on the lint command for this PR. I wonder if rebasing off the latest trunk might help. |
@Addison-Stavlo
|
Oh, that is a good question. Im not sure what makes the most sense: Navigation, Index, ListBox, something else? cc @Copons @david-szabo97 re: the wrapper for the navigation panel component. I also wonder why this wasn't an issue with the list-view component (perhaps b/c it has 'aria-labelledby'? or do we need to add a role there as well?). 🤔 |
@Addison-Stavlo here is all valid values for role attr - https://stackoverflow.com/questions/4170356/what-values-can-the-html5-xhtml-role-attribute-have |
@Addison-Stavlo , @Copons , @david-szabo97 new error : |
Maybe just add disable-lint comment at div tag ? |
yeah I think a lint disable (and not adding a role) might make the most sense in this specific circumstance: gutenberg/packages/edit-site/src/components/secondary-sidebar/list-view-sidebar.js Line 56 in 5ae2b1b
Apologies for not recommending that earlier, I should have noticed that 🤣 and realized that the role doesn't really make sense here. |
@Addison-Stavlo
` |
I reran the test and it passed. Looking at some recent commits on master, it seems to have failed for a few of them in the same manner. It seems something is inconsistent with that test setup that we will need to look into elsewhere. Anyways, this is working well and tests are passing. Thanks for all the work here! |
@Addison-Stavlo I am very happy ) thank you for help in this PR) |
Hello ) Just simple accessibility improvement to allow close nav panel of site editor on esc button. Issue - "Site Editor Navigation Panel - Allow 'esc' to close the panel. #29530"