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.

Sign up for GitHub

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

Jump to bottom

[base-ui][Switch] Add missing role attribute #40907

Merged
merged 9 commits into from
Feb 14, 2024
Merged

[base-ui][Switch] Add missing role attribute #40907

merged 9 commits into from
Feb 14, 2024

Conversation

KirankumarAmbati
Copy link
Contributor

@KirankumarAmbati KirankumarAmbati commented Feb 3, 2024 •
edited
Loading

Related to #40615

Changes in this PR:

Added the missing accessibility features for the Switch component and useSwitch hook:

  • Root element's role should be switch
  • Root element should have aria-checked attribute

@KirankumarAmbati KirankumarAmbati mentioned this pull request Feb 3, 2024
Closed
@KirankumarAmbati KirankumarAmbati marked this pull request as draft February 3, 2024 13:56
@KirankumarAmbati KirankumarAmbati marked this pull request as ready for review February 3, 2024 13:56
@mui-bot
Copy link

mui-bot commented Feb 3, 2024 •
edited
Loading

Netlify deploy preview

https://deploy-preview-40907--material-ui.netlify.app/

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 8f4894c

@oliviertassinari oliviertassinari added the package: base-ui Specific to @mui/base label Feb 3, 2024
oliviertassinari
oliviertassinari reviewed Feb 4, 2024
View reviewed changes
packages/mui-base/src/Switch/Switch.tsx Outdated
@@ -103,6 +103,9 @@ const Switch = React.forwardRef(function Switch<RootComponentType extends React.
elementType: Input,
getSlotProps: getInputProps,
externalSlotProps: slotProps.input,
additionalProps: {
role: 'switch', // Switch component should have the role "switch" to meet a11y guidelines
Copy link
Member

Choose a reason for hiding this comment

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

This should be applied directly in the JSX component, no?

Copy link
Contributor Author

@KirankumarAmbati KirankumarAmbati Feb 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tried adding the prop into useSwitch.getInputProps but useSwitch is being used by Radio, CheckBox along with Switch component. So, added it in additionalProps

Copy link
Member

Choose a reason for hiding this comment

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

This should be returned by the hook.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

useSwitch hook is being consumed by Radio and Checkbox components, along with the Switch component. Adding role = switch to the hook returned value breaks those components. To make the hook work, we might have send another parameter to useSwitch, which I felt isnt an ideal solution. We have additionalProps, which came handy for the fix.

But, I'm open for suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

useSwitch hook is being consumed by Radio and Checkbox components,

This is a bit unfortunate (and temporary). I'd make the hook return the correct set of props for the Switch and override them if needed in Checkbox and Radio button.
We are going to add Checkbox and Radio Button to Base UI in the next batch of components (likely late Q2 or Q3 this year), so it won't be an issue anymore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, thanks for the update @michaldudak! Made changes as per the discussion

michaldudak
michaldudak reviewed Feb 9, 2024
View reviewed changes
packages/mui-joy/src/Checkbox/Checkbox.tsx Outdated Show resolved Hide resolved
packages/mui-joy/src/Radio/Radio.tsx Outdated Show resolved Hide resolved
KirankumarAmbati and others added 2 commits February 9, 2024 19:04
@KirankumarAmbati @michaldudak
Update packages/mui-joy/src/Checkbox/Checkbox.tsx
085f8e4 
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Kirankumar Ambati <kiran.chinna12520@gmail.com>
@KirankumarAmbati @michaldudak
Update packages/mui-joy/src/Radio/Radio.tsx
a54f262 
Co-authored-by: Michał Dudak <michal.dudak@gmail.com>
Signed-off-by: Kirankumar Ambati <kiran.chinna12520@gmail.com>
@KirankumarAmbati
Copy link
Contributor Author

@michaldudak the build step test_e2e_website is failing but sems like unrelated to the PR. Please can you verify & retrigger the step, if that fixes that issue. Thanks!

@KirankumarAmbati
Copy link
Contributor Author

@michaldudak Triggered an empty build and pipeline is green now :)

@danilo-leal danilo-leal added the component: switch This is the name of the generic UI component, not the React module! label Feb 12, 2024
michaldudak
michaldudak approved these changes Feb 14, 2024
View reviewed changes
Copy link
Member

@michaldudak michaldudak left a comment

Choose a reason for hiding this comment

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

Looks good! Please write a summary of changes in the description of this PR so it's easier to find in the future.

@KirankumarAmbati
Copy link
Contributor Author

Looks good! Please write a summary of changes in the description of this PR so it's easier to find in the future.

Done @michaldudak 👍

@michaldudak michaldudak changed the title [base-ui][Switch] Missing accessibility features [base-ui][Switch] Add missing role attribute Feb 14, 2024
@michaldudak michaldudak merged commit fb3dfb0 into mui:master Feb 14, 2024
20 checks passed
@michaldudak
Copy link
Member

Thanks for your contribution!

@michaldudak michaldudak linked an issue Feb 16, 2024 that may be closed by this pull request
[base-ui][Switch] Missing accessibility features #40615
Closed
This was referenced May 24, 2024
Open
Open
@Crushoverride007 Crushoverride007 mentioned this pull request Jun 5, 2024
Open
This was referenced Jul 16, 2024
Open
Open
Open
@Crushoverride007 Crushoverride007 mentioned this pull request Jul 27, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oliviertassinari oliviertassinari oliviertassinari left review comments

@michaldudak michaldudak michaldudak approved these changes

Assignees
No one assigned
Labels
accessibility a11y component: switch This is the name of the generic UI component, not the React module! package: base-ui Specific to @mui/base
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[base-ui][Switch] Missing accessibility features
5 participants
@KirankumarAmbati @mui-bot @michaldudak @oliviertassinari @danilo-leal