-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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
[material-ui][Switch] Convert to support CSS extraction #41367
Conversation
Netlify deploy previewhttps://deploy-preview-41367--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
Argos is failing, the variants conversion is somehow wrong. |
Struggled a bit to find why it's failing in the docs and not in the app. Seems it's because I verified that - .filter(([, value]) => value.main && value.mainChannel && value.light) // check all the used fields in the style below
+ .filter(([, value]) => value.main && value.light) // check all the used fields in the style below |
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.
The styles for disabled
looks wrong from argos.
}, | ||
[`&.${switchClasses.disabled}`]: { |
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.
@@ -224,6 +237,7 @@ const Switch = React.forwardRef(function Switch(inProps, ref) { | |||
checkedIcon={icon} | |||
ref={ref} | |||
ownerState={ownerState} | |||
color={color} |
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.
color={color} |
No need to pass color as a prop here, the ownerState
is enough.
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.
legacy of the debug session 🙈
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.
👍 Nice!
backgroundColor: 'transparent', | ||
variants: [ | ||
...Object.entries(theme.palette) | ||
.filter(([, value]) => value.main && value.light) // check all the used fields in the style below |
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 line broke our production code. We have an entry in the theme object that is sometimes undefined. This code doesn't handle that and assumes all values should be an object (which they are typically), but can also be numbers and strings. It doesn't fail there, but when the value is undefined
.
.filter(([, value]) => value.main && value.light) // check all the used fields in the style below | |
.filter(([, value]) => value?.main && value?.light) // check all the used fields in the style below |
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.
Noted. will fix it soon.
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.
Could you create a new issue so that I can assign to myself?
Part of #41273
The customization demo is broken, because styles does not apply on the root component.
For example in the ios switch example, the
width
,height
andpadding
are not applied. But other classes are working fine 🤔About the
overridesResolver
I'm not sure about how the newstyled
react to it. It's the first time I see it used with an array