-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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
fix: Button accessibility #7420
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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.
LGMT ! Maybe add more unit test
yeah this PR had me hunting for why we changed it recently in the first place. making sure we are not ping-ponging use cases in a shared component is important |
I see you saying LGMT, but I bet you meant LGTM, no? |
Lighthouse Results
|
Description
In the
Button
component, the native event listener provided by the browser for anchors is modified, so in some cases it does not work as it shouldFor example, the download button on the home page with a tab and press enter does not start the download
With this PR, I changed the current approach a little bit so that if there is a
href
in theprops
, the button acts like ananchor
, and if there is nohref
, it acts like abutton
Validation
The places where the
Button
component is used in the preview must be accessibleRelated Issues
related to #7390, #7287, #7247
Check List
npm run format
to ensure the code follows the style guide.npm run test
to check if all tests are passing.npx turbo build
to check if the website builds without errors.