-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Switch] Create SwitchUnstyled and useSwitch #26688
Conversation
Details of bundle changes (experimental) @material-ui/core: parsed: +0.55% , gzip: +0.59% |
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.
Initial review. If we go ahead with this notion of introducing hooks, I think it's interesting to read trough this - https://kentcdodds.com/blog/the-state-reducer-pattern-with-react-hooks
packages/material-ui-unstyled/src/ButtonUnstyled/ButtonUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
Great initiative to open a draft so we can discuss on the changes early. The first thing that got my attention is regarding the constraints. The existing components aren't using the new unstyled ones (yet?). What's the plan regarding this aspect? I can think of two different approaches:
It seems that we are going with option 2. In the two cases, one big challenge is which level we extract/abstract. It has to be enough abstracted to make it valuable for developers (solve problems for them, otherwise they will write their own hook). It also has to be low level enough to allow us to implement a second design specification |
My next goal will be changing the Core
I chose to build the components from scratch using the existing functionality as reference. Of course I copied a lot of code from the existing components. I find it easier to start with a blank slate and add things when I see they are necessary, than start from a full-blown component and remove things that are not needed. Additionally, I wanted to build these components in TypeScript from the start.
I agree - it's hard to draw a line properly. For sure, all the accessibility features and nothing Material-specific should be in unstyled. |
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/ButtonUnstyled/ButtonUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/ButtonUnstyled/ButtonUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/ButtonUnstyled/ButtonUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
@michaldudak This sounds like a great next step. |
Agree, ideally we should not need to change any tests :) |
2c18d8a
to
9ca3348
Compare
I implemented the Switch component using the Inspired by #26303 I decided to remove the ButtonBase from the render tree. FocusVisible support is provided by |
@michaldudak It looks great 👌✨ |
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.
Looks pretty close to be ready for merge :)
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/SwitchUnstyled.tsx
Outdated
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/switchUnstyledClasses.ts
Show resolved
Hide resolved
packages/material-ui-unstyled/src/SwitchUnstyled/switchUnstyledClasses.ts
Show resolved
Hide resolved
@oliviertassinari, @mnajdova I addressed all the comments you made. Could you please take a final look at the PR and see if there's anything outstanding from your point of view? |
} = props; | ||
|
||
const Root: React.ElementType = component ?? components.Root ?? 'span'; | ||
const rootProps: any = { ...getForwardableProps(Root, otherProps), ...componentsProps.root }; |
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.
Why is this not enough?
const rootProps: any = { ...getForwardableProps(Root, otherProps), ...componentsProps.root }; | |
const rootProps: any = { ...otherProps, ...componentsProps.root }; |
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.
If Root
is a plain HTML element, I don't want to forward anything that doesn't apply to a HTML element.
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 mean, I don't get why "foo" shouldn't warn when it does with the buttons.
import * as React from "react";
import Button from "@material-ui/core/Button";
import SwitchUnstyled from "@material-ui/unstyled/SwitchUnstyled";
export default function UnstyledSwitches() {
return (
<div>
<SwitchUnstyled foo />
<Button foo />
<button foo />
</div>
);
}
https://codesandbox.io/s/unstyledswitches-material-demo-forked-rsrd1?file=/demo.tsx
The developers can swallow the extra prop. The new logic only check it with the "other" props on the root element, but not with the componentsProps
slots.
We didn't have the logic up until, it didn't seem to be a pain point. What use case do you have in mind?
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.
My thinking was that forwarding props from the host to the root is less explicit than just setting them in componentsProps.root
and therefore it may need some additional checking. If someone explicitly passes something in componentsProps.*
, I assume they know what they are doing.
This has an inconvenience of being inconsistent, though. I'm not particularly attached to this piece of code and if you feel we should always forward everything, so be 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.
cc @mui-org/core for more perspectives on the matter.
Outside of this point, the PR is good to go, from my side.
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 would forward all props, as it was done before. Let's not complicate the code for problems we haven't encounter so far.
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.
One tiny last comment for improving the demo. Let's also resolve #26688 (comment) before merging. Well done 🚀
const SwitchThumb = ({ isChecked, icon, checkedIcon, className }) => { | ||
if (!isChecked && icon) { | ||
return icon; | ||
} | ||
|
||
if (isChecked && checkedIcon) { | ||
return checkedIcon; | ||
} | ||
|
||
return <DefaultSwitchThumb className={className} />; | ||
}; |
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.
const SwitchThumb = ({ isChecked, icon, checkedIcon, className }) => { | |
if (!isChecked && icon) { | |
return icon; | |
} | |
if (isChecked && checkedIcon) { | |
return checkedIcon; | |
} | |
return <DefaultSwitchThumb className={className} />; | |
}; | |
const SwitchThumb = styled(({ isChecked, icon, checkedIcon, className }) => { | |
if (!isChecked && icon) { | |
return icon; | |
} | |
if (isChecked && checkedIcon) { | |
return checkedIcon; | |
} | |
return <span className={className} />; | |
}, { | |
name: 'MuiSwitch', | |
slot: 'Thumb', | |
overridesResolver: (props, styles) => styles.thumb, | |
})(({ theme }) => ({ | |
boxShadow: theme.shadows[1], | |
backgroundColor: 'currentColor', | |
width: 20, | |
height: 20, | |
borderRadius: '50%', | |
})); |
How about we merge this component with the DefaultSwitchThumb
? We would avoid having to think about names for components :)
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 isn't equivalent. icon
and checkedIcon
shouldn't be styled by us.
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.
What do you mean? They are not, they would be styled if we add the className
prop no?
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.
In your suggestion, if Sorry, I made wrong assumption about how icon
or checkedIcon
is provided, they would have the styles applied (boxShadow, backgroundColor, etc.), which is not what we wantstyled
works. You're right. I'll correct it.
@mui-pr-bot use i am import { Switch } from "@mui/material"; I have this issue. I don't know what issue. above image switch casse css reflet but When I refresh page it gets fixed |
@khushalsrashtasoft if you're experiencing problems with the Switch, please open a new issue and provide reproduction steps. |
This is an implementation of the SwitchUnstyled component and the useSwitch hook. One chunk of #6218
Summary
useSwitch
is a low-level utility for both internal and external use when maximum customizability is required. It provides an object to be used asinput
propsSwitchUnstyled
creates a structure of components and usesuseSwitch
internally.Root
,Thumb
andInput
components are overridable viacomponents
prop (andcomponent
, being equivalent tocomponents.Root
).useTouchRipple
hook.componentsProps
, but after encountering problems with PropTypes, I created a separate issue for this: [typescript] Better type annotations for unstyledcomponents
andcomponentsProps
props #26810describeConformanceUnstyled
. It executes the existing tests that apply to unstyled components + a few new ones (testingcomponents
andcomponentsProps
props)Preview: https://deploy-preview-26688--material-ui.netlify.app/components/switches/#unstyled-switches
To do