-
Notifications
You must be signed in to change notification settings - Fork 538
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
Revert "Add loading
prop for Button
and IconButton
(#3582)"
#4464
Conversation
|
size-limit report 📦
|
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.
Approved since it's a revert
loading
prop forButton
andIconButton
#3582 by reverting commit 3be64c5Sorry @langermank! Found some more bugs in integration tests that I wasn't able to fix inline
Bugs spotted in CI failures:
loading
prop forButton
andIconButton
#3582, we introduced an aria-labelledby for all instances of Button. This however isn't a good idea when aria-label is already present on the instance because aria-labelledby takes precedence over aria-label (we only want to add a fallback aria-labelledby when aria-label isn't present) (see CI error)loading
prop forButton
andIconButton
#3582 example: github/github/CommandButton (see CI error)Rollout strategy
Next steps:
Follow this guide to run integration tests for your pull request in dotcom: https://github.com/github/primer-engineering/blob/main/how-we-work/testing-primer-react-pr-at-dotcom.md