Skip to content
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.

Sign up for GitHub

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

fix(a11y): move aria-label from svg icon to button #2640

Merged
merged 5 commits into from
Nov 15, 2023
Merged

fix(a11y): move aria-label from svg icon to button #2640

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
afdfe86
fix(a11y): move aria-label from svg icon to button
seevee Oct 24, 2023
cf2344f
fix(a11y): improve aria-label accessibility
seevee Nov 14, 2023
889b985
fix(a11y): add close icon alt text
seevee Nov 14, 2023
e30c3cd
Revert "fix(a11y): add close icon alt text"
seevee Nov 14, 2023
0217733
Merge branch 'main' into cv-nav-button-aria-label-2639
brandonlenz Nov 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/components/header/NavCloseButton/NavCloseButton.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ export const NavCloseButton = ({
className={classes}
onClick={onClick}
data-testid="navCloseButton"
aria-label="Close Navigation Menu"
{...buttonProps}
type="button">
<Icon.Close size={3} aria-label="Close" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is an acceptable implementation to me, my ANDI output (which shows the text data that is received by screen readers visually) shows the close button labelled as 'Close' as intended.

image

However, I would encourage you to get more specific in the accessible name. Let's say we had another component in a page layout that also had a close icon. If both are named "close" they would be indistinguishable for a screen reader user navigating by links on a page.

the aria label is correct but I would change it to "close menu" or "close navigation" to avoid duplicate accessible names. If there was two components using close buttons under this pattern in the same page layout, we would see this ANDI error: ⚠ Non-unique button: same name/description as another button.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about enabling the prop to be passed in?

Whatever default value we provide here, whether "close" or "close menu" might be nice to have the option to override it. That would require an iconProps param passed in where we could then get iconProps['aria-label'], and if present, use that instead of the default.

I'm fine with that being an enhancement later on too though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shkeating thanks for the review and the advice! I've updated the aria-label to "Close Navigation Menu" to make it more unique and descriptive.

@brandonlenz Allowing the label to be passed in as a prop sounds like the ideal path to me! I think this would be especially useful if we want to show localized aria labels.

I think there's a couple of other components that could use that enhancement as well, so I'll keep the diff on this PR as lean as possible for now and open a new PR targeted at using those props as overrides. Let me know if that works!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That works for me.

<Icon.Close size={3} aria-hidden="true" />
</button>
)
}