-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
[A11y] refactor(ProductList): improve structure and accessibility #9795
[A11y] refactor(ProductList): improve structure and accessibility #9795
Conversation
✅ ethereum-org-website-dev deploy preview ready
|
d04dd48
to
6377bc0
Compare
src/components/ProductList.tsx
Outdated
@@ -22,10 +31,13 @@ export interface IProps { | |||
const ProductList: React.FC<IProps> = ({ content, category }) => { | |||
const shadow = useColorModeValue("tableBox.light", "tableBox.dark") | |||
|
|||
const ariaLabelledby = "cat-name" |
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 just had a question abut this label. Im a little confused what "cat-name" is. Should the be "category-name", or should we actually be passing the category prop instead?
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 just an id because it is being used in two places (lines 40 and 51) for the aria-labelledby
feature.
I can be clearer here in using a more verbose id, along with uppercase variable naming to denote this as a const variable 😁
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.
Sure! I pushed those changes up. This should be good to go now :)
Description
Improves accessibility navigating the
ProductList
component with a screen reader.List
andListItem
components.aria-labelledby
to ensure the user knows what the list should they skip the headingSecondary addition to the PR
This PR also updates the layout to provide more consistency with spacing.
cc @nloureiro for approval on this
Related Issue
Closes #8878