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

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
85aedd9
[base-ui][Switch] Missing accessibility features
KirankumarAmbati Feb 3, 2024
354030f
[base-ui][Switch] fixed failing test
KirankumarAmbati Feb 3, 2024
5633626
[base-ui][Switch] fixed failing tests
KirankumarAmbati Feb 3, 2024
b9b6c02
[base-ui][Switch] undo test changes
KirankumarAmbati Feb 3, 2024
b6b08cf
[base-ui][Switch] changes to return role from hook
KirankumarAmbati Feb 8, 2024
cebb6b7
[base-ui][Switch] fixed tests
KirankumarAmbati Feb 8, 2024
085f8e4
Update packages/mui-joy/src/Checkbox/Checkbox.tsx
KirankumarAmbati Feb 9, 2024
a54f262
Update packages/mui-joy/src/Radio/Radio.tsx
KirankumarAmbati Feb 9, 2024
8f4894c
Trigger Build
KirankumarAmbati Feb 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/mui-base/src/Switch/Switch.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

},
ownerState,
className: classes.input,
});
Expand Down
1 change: 1 addition & 0 deletions packages/mui-base/src/useSwitch/useSwitch.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ export function useSwitch(props: UseSwitchParameters): UseSwitchReturnValue {
ref: handleInputRef,
required,
type: 'checkbox',
'aria-checked': checkedProp,
...otherProps,
onChange: createHandleInputChange(otherProps),
onFocus: createHandleFocus(otherProps),
Expand Down
Loading