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: add aria label and aria disabled #691

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

flavia-moraes
Copy link
Collaborator

JIRA Issue

Description 📄

Add accessible attributes: aria-disabled and aria-label

Platforms 📲

  • Web
  • Mobile

Type of change 🔍

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested? 🧪

[Enter the tips to test this PR]

  • Unit Test
  • Snapshot Test

Checklist: 🔍

  • My code follows the contribution guide of this project Contributing Guide
  • Layout matches design prototype: FIGMA
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@@ -30,6 +31,8 @@ const Button = forwardRef(
<StyledButton
ref={ref}
disabled={disabled}
aria-disabled={disabled || undefined}
Copy link
Contributor

@MicaelRodrigues MicaelRodrigues Sep 25, 2023

Choose a reason for hiding this comment

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

Isn't this enough?

Suggested change
aria-disabled={disabled || undefined}
aria-disabled={disabled}

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did a test to see what the difference would be when disabled is false or undefined

With: aria-disabled={disabled || undefined}
Screenshot 2023-09-26 at 10 50 51

And with: aria-disabled={disabled}
Screenshot 2023-09-26 at 10 51 28

Using aria-disabled={disabled} the aria-disabled attribute receives false, on the other hand, using aria-disabled={disabled || undefined} the aria-disabled attribute does not even appear.

I'm thinking what the best way would be, what do you think @MicaelRodrigues?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@MicaelRodrigues I talked to the team and we realized that it would be good to keep aria-disabled={disabled || undefined}.

It's more common the aria-disabled not to appear when it's false

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a test to see what the difference would be when disabled is false or undefined

With: aria-disabled={disabled || undefined} Screenshot 2023-09-26 at 10 50 51

And with: aria-disabled={disabled} Screenshot 2023-09-26 at 10 51 28

Using aria-disabled={disabled} the aria-disabled attribute receives false, on the other hand, using aria-disabled={disabled || undefined} the aria-disabled attribute does not even appear.

I'm thinking what the best way would be, what do you think @MicaelRodrigues?

@MicaelRodrigues I talked to the team and we realized that it would be good to keep aria-disabled={disabled || undefined}.

It's more common the aria-disabled not to appear when it's false

@matheushdsbr I understood all that... ;)
but if we want the default to be undefined we should just update the default prop value for disabled to undefined.

Take a look at the chat bellow. There are slight differences between false and undefined use cases. A Button user may want to explicitly set aria-disabled to false and with the current implementation he can't.

https://chat.openai.com/c/12783a4b-bd20-4823-bc2c-62b4fdb4f2f7

WDYT? :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the tips, let's continue with changing disabled in defaultProps to undefined

@matheushdsbr matheushdsbr merged commit 9591ea3 into master Oct 2, 2023
3 checks passed
@matheushdsbr matheushdsbr deleted the feat/web-button-acessibility branch October 2, 2023 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants