[muiStyled] Support default theme when none is available #22791
Conversation
|
@material-ui/core: parsed: +0.25% , gzip: +0.26% |
| @@ -1 +1,2 @@ | |||
| export { default } from '@emotion/styled'; | |||
| export * from 'emotion-theming'; | |||
There was a problem hiding this comment.
We should specify what we need in case emotion-theming adds new modules that we don't need.
There was a problem hiding this comment.
What problem does emotion-theming solve?
There was a problem hiding this comment.
By adding the emotion's/styled-components' ThemeProvider in our ThemeProvider component, developers no longer need to specify ThemeProvider from emotion/styled component together with ours. Also, in the styled utility when custom theme is used, it will automatically be available on props.theme. In addition to this, when no ThemeProvider is used, the styled utility uses the defaultTheme
There was a problem hiding this comment.
What if they already use emotion-theming?
There was a problem hiding this comment.
I think that using a different ThemeProvider (enforcing the usage of two) makes incremental adoption easier. Developers don't have to worry about conflicts of theme structure with existing usage of emotion/sc.
However, for using the css prop, it seems required https://emotion.sh/docs/theming#css-prop.
It seems that theme-ui imports the ThemeContext of emotion directly (without emotion-theming), https://github.com/system-ui/theme-ui/blob/af57d4ce97c0e8a3289115f33cef1d17bc65adad/packages/core/src/index.ts#L3. They also have emotion as a direct dependency.
There was a problem hiding this comment.
Just a clarification, for our components, like Slider, Button, muiStyled + our ThemeProvider is working, it won't work for the scenarios of using muiStyled + div, span or other primitive elements as we do not have access to change the behavior there.
There was a problem hiding this comment.
Oh, I see, thanks for the clarification. +1 for including the styled-engine theme provider in Material-UI's ThemeProvider component. It's simpler. For the theme structure conflict, developers can workaround the issue by nesting <MuiThemeProvider> > <Emotion ThemeProvider>.
The only point I would suggest to explore is: Using the ThemeContext exported from @emotion/core vs. emotion-theming.
There was a problem hiding this comment.
The only point I would suggest to explore is: Using the ThemeContext exported from @emotion/core vs. emotion-theming.
Yeah, let me spend some time on this, just to make sure I am not missing something 👍
There was a problem hiding this comment.
Refactored to use only the ThemeContext (both emotion & styled-components expose this). Tested with both engines, tests are passing, so I think everything is covered. No new dependencies added with this 👍
There was a problem hiding this comment.
No new dependencies added with this
Nice :).
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
mnajdova
changed the title
There was a problem hiding this comment.
I think that it would be been better to name the new helper experimentalStyled, as Sebastian proposed, so we could iterate on it, without breaking developers' using v5-alpha code until we find something we happy with. It also removes the need to consider the migration story from the changes here, to be handled later on.
Let's do that, I have too many changes on this PR that are not really related to the problem I am solving. We can do the rename/deprecation later on. |
|
|
||
| // with yarn | ||
| yarn add @material-ui/styles | ||
| yarn add @material-ui/styles @emotion/core @emotion/styled |
There was a problem hiding this comment.
Does the styles package really need these new dependencies? I was under the assumption that we would try not to interfer with it, progressively making it legacy, and dropping it.
There was a problem hiding this comment.
The only reason there were added is because the ThemeProvider is in this package. Once we move it out of the package we can drop these dependencies
There was a problem hiding this comment.
We only document the import of ThemeProvider from the core. Would it work if we moved the theme provider nesting to the core?
My concern is about the migration experience for v4 developers that are using JSS. We could imagine having developers migrate their v4 imports from @material-ui/core/styles to @material-ui/styles and have still everything working. We could no longer have @material-ui/styles as a dependency in @material-ui/core. Would that work? Do you have an alternative story in mind? A migration from @material-ui/core/styles to react-jss won't be as smooth.
There was a problem hiding this comment.
Ah I see what is the concern. Let me try then to migrate the nesting to the core 👍
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
Co-authored-by: Matt <github@nospam.33m.co>
Co-authored-by: Matt <github@nospam.33m.co>
| export { default as styled, ComponentCreator, StyledProps } from './styled'; | ||
| export { | ||
| default as MuiThemeProvider, |
There was a problem hiding this comment.
We have stopped documenting MuiThemeProvider in v4. I would propose we keep pushing in the same direction: we drop MuiThemeProvider in v5. The motivation for using ThemeProvider over MuiThemeProvider is to signal that a developer only needs one theme provider on the page, not one from Material-UI and one from styled-components. I have created https://trello.com/c/q6cdyyaP/2744-remove-muithemeprovider to keep track of this proposal.
zannager
added
the
package: styles
This PR adds support for default theme in
muiStyled, utility in@material-ui/core.In addition to this,
ThemeContextis exported from both@material-ui/styled-engineand@material-ui/styled-engine-sc, which is used in ourThemeProvider. This simplifies the DX when usingmuiStyled, in means that developers no longer need to add in addition to ourThemeProvidertheThemeProviderfrom their styled engine -emotionorstyled-components.