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.

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

feat: allow a node to be passed in as an IconButton label #643

Merged
merged 4 commits into from
May 4, 2022

Conversation

thuey
Copy link
Collaborator

@thuey thuey commented May 3, 2022

This allows a node to be passed in as the label for an IconButton. Allowing a node to be passed in allows consumers to include things like screen reader-only content. (see the docs example in this PR)

@thuey thuey requested a review from a team as a code owner May 3, 2022 19:53
@github-actions
Copy link
Contributor

github-actions bot commented May 3, 2022

schne324
schne324 previously approved these changes May 3, 2022
Copy link
Member

@scurker scurker left a comment

Choose a reason for hiding this comment

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

I don't think this will work as implemented, see how we're also setting aria-label below:

useEffect(() => {
if (!disabled) {
return;
}
internalRef.current?.setAttribute('aria-label', label);
}, [disabled]);

I think you're on the right track, but we also need the button to have an accessible name as well. Perhaps we should be using <Offscreen> there instead of just using aria-label.

@thuey
Copy link
Collaborator Author

thuey commented May 4, 2022

@scurker Good catch. I missed the "disabled" scenario.

@thuey thuey requested a review from scurker May 4, 2022 18:36
@thuey thuey merged commit cd53f1d into develop May 4, 2022
@thuey thuey deleted the allow-react-node-icon-button-label branch May 4, 2022 20:55
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2022

@scurker scurker mentioned this pull request May 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants