Icon button fixes: Removes iconLabel and adds aria-label to the type #1945
Conversation
🦋 Changeset detectedLatest commit: 93a508b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
src/Button/types.ts
Outdated
| @@ -45,7 +45,7 @@ export type IconButtonProps = { | |||
| * This is to be used if it is an icon-only button. Will make text visually hidden | |||
There was a problem hiding this comment.
do you need this comment anymore?
src/Button/types.ts
Outdated
| @@ -45,7 +45,7 @@ export type IconButtonProps = { | |||
| * This is to be used if it is an icon-only button. Will make text visually hidden | |||
| */ | |||
| icon: React.FunctionComponent<IconProps> | |||
| iconLabel: string | |||
| 'aria-label': string | |||
There was a problem hiding this comment.
Should be this required? Also, what would the user do if they wanted to apply aria-labelledby instead, should we have some logic to only apply the aria-label only if aria-labelledby hasn't been passed in so that takes precedence?
There was a problem hiding this comment.
Related: What's the right way to add a tooltip to an IconButton, like the one on a file?
When composable Tooltip + IconButton, does the aria-label go on both of them or just one?
This would be useful for trailing action for TextInput (https://github.com/github/primer/issues/748)
There was a problem hiding this comment.
@siddharthkp - I actually think we'd want the aria-label on the button but not the tooltip.
Here's what I think would give a nice experience for screenreaders
When the tooltip is acting like a label
<button aria-label="Edit this file"><svg>{/* paths and stuff */}</svg></button>
<div>Edit this file</div> // <--- this is a tooltipOR
<button aria-labelledby="editTooltip"><svg>{/* paths and stuff */}</svg></button>
<div id="editTooltip">Edit this file</div> // <--- this is a tooltipWhen the tooltip is acting like helper/caption text
<button aria-label="Edit this file" aria-describedby="editTooltip"><svg>{/* paths and stuff */}</svg></button>
<div id="editTooltip">Only people in this organization can edit this file</div> // <--- this is a tooltip@alliethu - please keep me correct
There was a problem hiding this comment.
Also, I just realized our tooltips aren't displayed on focus, only hover. :(
We probably need an a11y audit on these
There was a problem hiding this comment.
@mperrotti Yes, the aria-label should be on the button. Also, heads-up that @khiga8 just did a recent release to fix tooltips on our comment box toolbars. Hopefully, you should be able to reference her implementation for the fixes on the PRC end.
https://github.com/github/accessibility/issues/594#issuecomment-1034219464
There was a problem hiding this comment.
@mperrotti even more current, @hectahertz is working on Upstreaming tooltip to PVC. The two of you should coordinate! https://github.com/github/accessibility/issues/802
There was a problem hiding this comment.
Here is the upstreamed tooltip component. The tooltip is implemented as a custom element. You may refer to implementation custom element here.
There was a problem hiding this comment.
For this PR, I'll try and support both aria-label and aria-labelled-by in the types. Hope that works 🤞
| @@ -91,8 +91,9 @@ describe('Button', () => { | |||
| }) | |||
|
|
|||
| it('styles icon only button to make it a square', async () => { | |||
| const container = render(<IconButton icon={SearchIcon} iconLabel="Search icon only button" />) | |||
| const container = render(<IconButton icon={SearchIcon} aria-label="Search button" />) | |||
There was a problem hiding this comment.
I think we should have a separate test for the aria-label / aria-labelledby. thoughts?
also a suggestion for your matcher, to be more specific around looking for the aria-label attributes:
- findByRole('button')
+ getByRole('button', {name: 'Search button'} ) I don't think you need use an async matcher? 🤔
| const IconOnlyButton = await container.findByRole('button') | ||
| expect(IconOnlyButton).toHaveStyleRule('padding-right', '8px') | ||
| expect(IconOnlyButton).toMatchSnapshot() |
There was a problem hiding this comment.
I feel like aria attributes fall under logic, so I think it's worth adding a separate test for it like..
expect(IconOnlyButton).toHaveAttribute('aria-label', labelThingy)
There was a problem hiding this comment.
Can we update the first example on the docs as well to use aria-label instead of children: IconButton#icon-only-button and on the page for Button: Button#icon-only-button
sorry for not catching this one on the first pass!
we can also remove the "drafts" meta string from code examples now ✨
| @@ -94,7 +94,7 @@ A separate component called `IconButton` is used if the action shows only an ico | |||
|
|
|||
| ```jsx live | |||
| <> | |||
| <IconButton aria-label="Search" size="small" icon={SearchIcon} /> | |||
| <IconButton aria-label="Search" size="small" icon={SearchIcon} /> | |||
Describe your changes here.
The documentation and stories for
IconButtonwere not consistent.The
iconLabelis not actually required.The
aria-labelshould be mandatory forIconButtonScreenshots
Please provide before/after screenshots for any visual changes
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.