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

Fix button focus #7287

Merged
merged 14 commits into from
Dec 16, 2024
Merged

Fix button focus #7287

merged 14 commits into from
Dec 16, 2024

Conversation

faridomarAf
Copy link
Contributor

@faridomarAf faridomarAf commented Nov 28, 2024

Button focusable issue fixed

Description

While we are using a Link-component for both Button & Link purposes, there should be a logic when it use as a Link it should acts as a Link, and when it use as Button it should as Button.

1: Enhancements to Button Component::

  • Role Handling: Added logic to differentiate between button and link usage based on the presence of the href prop.
  • Keyboard Accessibility: Implemented onKeyDown to handle Enter and Space keys, ensuring proper behavior for keyboard users.
  • Disabled State Management: Updated to respect disabled prop:
    • Prevented clicks and keyboard interaction when button disabled.
    • Removed from the tab order using tabIndex when disabled.
    • Applied a disabled style conditionally.
  • Cleaner Role Logic: Dynamically set role to button only when used as a button.
  • Improved Prop Handling: Ensured onClick and tabIndex behave appropriately based on component usage.

2: Enhancements to CodeBox Component:

  • Dynamic Button State:
    • Added a useState hook to manage the disabled state of the Copy button.
    • Used useEffect to dynamically update the button's disabled state based on the presence of content in the <pre> element.
  • Keyboard Accessibility:
    • Ensured proper tab behavior with tabIndex on the <pre> element for improved accessibility.
  • Improved User Experience:
    • Prevented users from interacting with the Copy button if no content is available to copy.

Validation

https://www.loom.com/share/6f237ae46e7945f48650c1e710399e6f?sid=f8f6c2fc-faca-46e8-901a-021c38f091f9

Related Issues

#7247

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

@faridomarAf faridomarAf requested a review from a team as a code owner November 28, 2024 18:26
Copy link

vercel bot commented Nov 28, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Dec 16, 2024 0:42am

Copy link

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@mikeesto mikeesto 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 understand why this component isn't a native <button> (there may well be a good reason). I think this change is an improvement because now it is focusable, but the default behaviour of href="#" is to jump the page to the top so that's not ideal either

Copy link
Member

@ovflowd ovflowd 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 is an appropriate fix, we should be using tabindex and other properties.

@ovflowd
Copy link
Member

ovflowd commented Nov 30, 2024

I don't understand why this component isn't a native <button> (there may well be a good reason). I think this change is an improvement because now it is focusable, but the default behaviour of href="#" is to jump the page to the top so that's not ideal either

There are numerous reasons, mostly because all our "buttons" are links, this one is deceptively the only one that is not a button. Per specification a a element with role button, for all intends and purposes is a button; So that's not the issue.

@faridomarAf
Copy link
Contributor Author

yeah guys this is not the proper way, sorry for late replying, you are right
let me check the components which this component is used in those because there are some functionality for "Enter key" and "Space key" that should be considered and also we should check is there any necessity for the "href" or no, whit checking all these, I would come whit a better solution for that which can be logically satisfying 😊

@faridomarAf
Copy link
Contributor Author

hey guys @mikeesto @AugustinMauroy @ovflowd @bjohansebas could you please review my code, if there is any suggestion PLZ 😊

Copy link
Member

@mikeesto mikeesto left a comment

Choose a reason for hiding this comment

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

Button focus seems good to me 👍

apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
Copy link

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

Everything seems to be working fine, LGTM

@bmuenzenmeyer
Copy link
Collaborator

@faridomarAf please resolve the CI failures if you have time

@ovflowd
Copy link
Member

ovflowd commented Dec 13, 2024

Apologies for not checking in here, have been busy with personal matters 🙏

@faridomarAf
Copy link
Contributor Author

faridomarAf commented Dec 14, 2024

hi @bmuenzenmeyer , sorry for late response, I think the changes which I made in Button.tsx file, it caused the error, so a I checked the original code that is working, I would do my best to solve the issue

Copy link
Contributor

github-actions bot commented Dec 14, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 90%
89.29% (634/710) 70.72% (186/263) 93.12% (122/131)

Unit Test Report

Tests Skipped Failures Errors Time
143 0 💤 0 ❌ 0 🔥 5.062s ⏱️

apps/site/components/Common/Button/index.tsx Show resolved Hide resolved
apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
apps/site/components/Common/Button/index.tsx Outdated Show resolved Hide resolved
Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM!

@ovflowd
Copy link
Member

ovflowd commented Dec 16, 2024

Thanks for the first time contribution! Excited to get this merged ✨

@ovflowd ovflowd enabled auto-merge December 16, 2024 12:44
@ovflowd ovflowd added this pull request to the merge queue Dec 16, 2024
Merged via the queue into nodejs:main with commit 3c274ba Dec 16, 2024
13 checks passed
@faridomarAf
Copy link
Contributor Author

hey guys thank you so much for your follow up, actually it was my fist time to contribute in an open source project. all your comments an code views values much more for me
thanks 🙏

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.

6 participants