-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(Button): add icon-only button support #3368
feat(Button): add icon-only button support #3368
Conversation
Deploy preview for carbon-components-react ready! Built with commit 7c4e7d2 https://deploy-preview-3368--carbon-components-react.netlify.com |
Deploy preview for the-carbon-components ready! Built with commit 7c4e7d2 https://deploy-preview-3368--the-carbon-components.netlify.com |
Deploy preview for carbon-elements ready! Built with commit 7c4e7d2 |
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.
Thanks for taking this work over fo the React demos!!
Button | ||
</button> | ||
</div> | ||
aria-label="danger" {{/if}} type="button"> |
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.
FYI, if you add an aria-label
to a button the SR doesn't actually read the contents of them button so the user would never hear Button
. I had a chat with @snidersd about this and she said that for danger there really isn't a way to denote this variant generically to the user but we be following best practices for content design and making sure the label of the button is clear to the user.
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.
thanks for the info, this was just carried over from our existing button examples. should we open a new PR to remove that aria-label
in all of the examples?
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 agree that it's beyond the scope of this PR and should be moved to a separate issue.
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.
LGTM 👍 - Thanks @emyarod for doing this!
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.
Nice work 👍
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.
Looks good! 🎉
Closes #2490
This PR adds support for icon-only buttons in the React library
Refs #2287, #2366, #3115
Changelog
New
Changed
Removed
Testing / Reviewing
Ensure that icon-only buttons appear and function as expected