-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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
[Joy] Add functional Switch
component
#30487
Conversation
Switch
componentSwitch
component
@mui/joy: parsed: +7.26% , gzip: +5.63% |
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.
Good work overall! I found just a few things to discuss.
name: 'MuiSwitch', | ||
slot: 'Track', | ||
overridesResolver: (props, styles) => styles.track, | ||
})<{ ownerState: SwitchProps & { focusVisible: boolean } }>(() => ({ |
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.
This reminds me I need to export the SwitchOwnerState interface to avoid construction of such types ad hoc.
<SwitchTrack ownerState={ownerState} className={clsx(classes.track)} /> | ||
<SwitchThumb ownerState={ownerState} className={clsx(classes.thumb)} /> | ||
<SwitchInput ownerState={ownerState} {...getInputProps()} className={clsx(classes.input)} /> |
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.
I see there's no ability to customize the props of inner components (componentsProps
is not exposed). Is it by design or do you plan to still add it?
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.
Good catch, I will add it.
return generateUtilityClass('MuiSwitch', slot); | ||
} | ||
|
||
const switchClasses: SwitchClasses = generateUtilityClasses('MuiSwitch', [ |
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.
IMO we should use a different prefix to avoid conflicts with @mui/material's Switch. It may be a rare use case to have both in the same project, but it doesn't cost us anything to prevent such problems.
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.
avoid conflicts with @mui/material's Switch
Do you mean when developers use both material and joy in the same project? In that case, they can do this:
import { switchClasses as materialSwitchClasses } from '@mui/material/Switch';
import { switchClasses as joySwitchClasses } from '@mui/joy/Switch';
unless I misunderstand the meaning of the different prefix
that you mentioned.
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.
No, I meant the MuiSwitch
prefix that gets added to CSS classes. If a project uses plain CSS to customize our components, it'll be impossible to differentiate between Material's Switch and Joy's Switch, as they both will have MuiSwitch-* classes.
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.
Ah, got it. I will change it in another PR for Switch, Button and Typography.
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.
👍
Preview: https://deploy-preview-30487--material-ui.netlify.app/experiments/joy/switch/
Summary
Review suggestion
docs/pages/experiments/joy/switch.tsx