Skip to content
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

[system][material-ui] Stabilize CssVarsProvider #41070

Closed
DiegoAndai opened this issue Feb 12, 2024 · 15 comments · Fixed by #42839
Closed

[system][material-ui] Stabilize CssVarsProvider #41070

DiegoAndai opened this issue Feb 12, 2024 · 15 comments · Fixed by #42839
Assignees
Labels
enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material package: system Specific to @mui/system

Comments

@DiegoAndai
Copy link
Member

DiegoAndai commented Feb 12, 2024

As we will rely heavily on component tokens with Material Design 3 implemented through CSS variables, we should stabilize the CssVarsProvider API and remove the "Experimental" flag.

Steps

v6

v7

  • Remove ThemeProvider

Search keywords: CssVarsProvider experimental stable

@DiegoAndai DiegoAndai added package: system Specific to @mui/system package: material-ui Specific to @mui/material enhancement This is not a bug, nor a new feature labels Feb 12, 2024
@DiegoAndai DiegoAndai added this to the Material UI: v7 (draft) milestone Feb 12, 2024
@DiegoAndai
Copy link
Member Author

DiegoAndai commented Feb 12, 2024

@mnajdova @siriwatknp @brijeshb42 what do you think about this one?

We could also plan to make the CssVarsProvider the only provider in v7. If we wish to do that, we would have to stabilize it in v6 as well as deprecate the ThemeProvider, marking it for removal in v7.

@oliviertassinari oliviertassinari changed the title [material-ui][mui-system] Stabilize CSSVarsProvider [material-ui][mui-system] Stabilize CssVarsProvider Feb 12, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 12, 2024

Involved discussion: #42887

@oliviertassinari oliviertassinari changed the title [material-ui][mui-system] Stabilize CssVarsProvider [mui-system][material-ui] Stabilize CssVarsProvider Feb 12, 2024
@danilo-leal danilo-leal changed the title [mui-system][material-ui] Stabilize CssVarsProvider [system][material-ui] Stabilize CssVarsProvider Feb 12, 2024
@siriwatknp
Copy link
Member

I agree to stabilize the API. The big task left is to handle the performance. createTheme takes ~0.x ms but extendTheme takes ~2-5ms.

The best way I found to optimize this (for MUI system) is that the extendTheme should be called at build-time via a CLI.

@brijeshb42
Copy link
Contributor

In that case, a user friendly way to do this would be to have a bundler plugin to inject the themes. It would be similar to a subset of what the zero runtime plugin does.

@DiegoAndai
Copy link
Member Author

I added an overview of the steps to the description. I also added it to the v6 milestone. It doesn't require breaking changes, but we have to deprecate ThemeProvider as soon as possible to give users time to adjust. Because of this, we'll schedule this work alongside the v6 work.

@DiegoAndai
Copy link
Member Author

DiegoAndai commented Feb 22, 2024

@siriwatknp, May I assign this one to you? You seem to have better context for it right now, having worked on the CssVarsProvider and the zero-runtime side.

@siriwatknp
Copy link
Member

siriwatknp commented Feb 23, 2024

@siriwatknp, May I assign this one to you? You seem to have better context for it right now, having worked on the CssVarsProvider and the zero-runtime side.

Yes, please assign it to me.

@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 24, 2024

I dove into the logic in: #41223.

  1. The behavior of the returned value by:
const { mode, systemMode } = useColorScheme();

is probably not right. I would expect these two should be undefined/null on the first render, unless { noSsr: true } is provided as an option. Otherwise, how are you supposed to handle hydration? The main downside is that it mean that the component renders twice, but this is more sound. And developers could still use { noSsr: true } like useMediaQuery provides.

  1. Also, I noticed that we don't memo the theme when we update the mode-related value. This means that we render all the descendants of that rely on the theme twice, this is horrible for performance. A reproduction: https://codesandbox.io/p/sandbox/gracious-currying-nxfvjd?file=%2Fsrc%2FDemo.tsx. Can someone create a dedicated GitHub issue for this? Thanks

Deprecate ThemeProvider in favor of CssVarsProvider

Are we sure about this? It doesn't sound compatible with keeping emotion running on the side.

@siriwatknp
Copy link
Member

Deprecate ThemeProvider in favor of CssVarsProvider

I don't think we need to deprecate ThemeProvider, it's still useful for testing purpose.

@DiegoAndai
Copy link
Member Author

It doesn't sound compatible with keeping emotion running on the side.

Why? The current CssVarsProvider works with Emotion.

I don't think we need to deprecate ThemeProvider, it's still useful for testing purpose.

The purpose of deprecating it would be to be able to eventually remove it, to avoid confusing users about which one they should use, and to avoid duplicating work on maintaining and supporting two different theme providers.

@DiegoAndai DiegoAndai moved this from Selected to In progress in Material UI May 24, 2024
@DiegoAndai
Copy link
Member Author

Hey @siriwatknp! I'm looking to organize the missing work to close this issue, on the weekly meeting we discussed with the team the following tasks:

  • Optimize extendTheme (comment). Is this still needed? Or was it fixed already?
  • Update the docs to replace ThemeProvider with CssVarsProvider
  • Deprecate ThemeProvider in favor of CssVarsProvider. We should have

The last two are to avoid users becoming confused with two providers. We want them to use CssVarsProvider.

Do you agree with these remaining tasks? Did I miss something? If you agree we can go ahead and create separate issues for them.

@siriwatknp
Copy link
Member

Optimize extendTheme (#41070 (comment)). Is this still needed? Or was it fixed already?

Pigment CSS fixes this at build time, so I think we can close the issue.

Update the docs to replace ThemeProvider with CssVarsProvider

Do you mean all of the places in the demos?

Deprecate ThemeProvider in favor of CssVarsProvider

Got it. Sounds good to me.

@DiegoAndai
Copy link
Member Author

Do you mean all of the places in the demos?

Yes

Got it. Sounds good to me.

Should we create separate issues for these?

@github-project-automation github-project-automation bot moved this from In progress to Done in Material UI Jul 18, 2024
@DiegoAndai
Copy link
Member Author

@siriwatknp should we go ahead with these:

  • Update the docs to replace ThemeProvider with CssVarsProvider
  • Deprecate ThemeProvider in favor of CssVarsProvider

If so, may I ask you to create issues for it?

@siriwatknp
Copy link
Member

@siriwatknp should we go ahead with these:

  • Update the docs to replace ThemeProvider with CssVarsProvider
  • Deprecate ThemeProvider in favor of CssVarsProvider

If so, may I ask you to create issues for it?

Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material package: system Specific to @mui/system
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants