-
-
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] Stabilize CssVarsProvider
and extendTheme
#42246
Conversation
Netlify deploy preview
@material-ui/core: parsed: +0.10% , gzip: +0.10% Bundle size reportDetails of bundle changes (Toolpad) |
docs/data/material/design-resources/material-ui-sync/material-ui-sync.md
Outdated
Show resolved
Hide resolved
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.
Left few small comments, from technical point of view it looks good 👍
docs/data/material/customization/css-theme-variables/overview.md
Outdated
Show resolved
Hide resolved
docs/data/material/customization/css-theme-variables/overview.md
Outdated
Show resolved
Hide resolved
docs/data/material/customization/css-theme-variables/usage/usage.md
Outdated
Show resolved
Hide resolved
docs/data/material/customization/css-theme-variables/usage/usage.md
Outdated
Show resolved
Hide resolved
docs/data/material/customization/css-theme-variables/usage/usage.md
Outdated
Show resolved
Hide resolved
docs/data/system/experimental-api/css-theme-variables/css-theme-variables.md
Outdated
Show resolved
Hide resolved
docs/data/system/experimental-api/css-theme-variables/css-theme-variables.md
Outdated
Show resolved
Hide resolved
docs/data/system/experimental-api/css-theme-variables/css-theme-variables.md
Show resolved
Hide resolved
@@ -4,7 +4,7 @@ import { | |||
demos, | |||
docs, | |||
demoComponents, | |||
} from 'docs/data/material/experimental-api/css-theme-variables/usage/usage.md?muiMarkdown'; | |||
} from 'docs/data/material/customization/css-theme-variables/customization.md?muiMarkdown'; |
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 don't love this path name: /material/customization/foobar/customization
just looks weird to me with customization
repeated. I'm not sure which one really needs to be replaced but it seems like this doc file name is the simpler choice.
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.
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 changed to "configuration", is it better?
docs/data/material/migration/migration-v5/migration-css-theme-variables.md
Outdated
Show resolved
Hide resolved
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.
Same as Marija, technical changes look good to me
Good job carrying this through the finish line 🚀
@siriwatknp this closes #41070 right? |
ad5df08
to
6156593
Compare
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.
🚢 it!
What do you think about renaming At this point, there is no reason not to move CSS variables all-in. Keeping a hybrid mode brings no value beyond allowing people to migrate. But even then it can very likely be done with barely any breaking changes. I mean, why would there be any? So I would deprecate On the Pigment CSS, side, I think that we could already rename to On a different topic, looking at the source, I'm confused about the architecture. I would propose we create 7 issues, one for each:
Why don't we straight forward the theme id? Why do we check if the theme exists before forwarding it? I mean, wouldn't it be simpler and more predictable to have
Couldn't we remove those?
Could we remove all of this? Place them into a dedicated structure, or as arguments to the theme creator.
|
Changes