[system] Support variants in theme.styleOverrides
#40690
Conversation
siriwatknp
added
package: system
Netlify deploy previewhttps://deploy-preview-40690--material-ui.netlify.app/ @material-ui/core: parsed: -0.17% 😍, gzip: -0.19% 😍 Bundle size reportDetails of bundle changes (Toolpad) |
|
|
||
| if (componentName && overridesResolver) { | ||
| expressionsWithDefaultTheme.push((props) => { | ||
| const theme = resolveTheme({ ...props, defaultTheme, themeId }); | ||
| const styleOverrides = getStyleOverrides(componentName, theme); |
There was a problem hiding this comment.
getStyleOverrides is not reused, so remove it to not shift between functions.
…em/style-overrides-variants
| styledArgVariants, | ||
| ); | ||
| variantStyles.forEach((variantStyle) => { | ||
| result = deepmerge(result, variantStyle); |
There was a problem hiding this comment.
I just noticed this deepmerge. I think we should remove it and let emotion/styled-components handle it. Both of them already support array of styles.
@mnajdova Should we try to remove it? the perf should be a bit better while producing the same result.
There was a problem hiding this comment.
Hmm, would it work if we remove it? From what I remember some of these definitions are merged into one style arg, this is what I remember.
There was a problem hiding this comment.
Yes, it would work but there is a tiny difference in the dev tool
without merging style, you will see all the styles declared in the variants when props are matched:
.hashed-variant2:hover {
color: red;
}
.hashed-variant1:hover {
color: green;
}but the results (cascading) are the same.
There was a problem hiding this comment.
I'd not merge so that the logic does not depend on the deepmerge but rely on the style engine.
There was a problem hiding this comment.
Have we tried removing the deepmerge and see if some test are failing?
…em/style-overrides-variants
…em/style-overrides-variants
Motivation
Result
Reduced ~120 lines of duplicate code by moving inside one
processStyleArgfunction which makestheme.styleOverrides.*.variantswork.Next step
Consider deprecating the
theme.variantsAPI in #30412 and add TODO to remove it in the next major version.