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

[core] Replace CssVarsProvider with ThemeProvider #3872

Merged
merged 17 commits into from
Aug 8, 2024
Merged

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 1, 2024

To test mui/material-ui#43115 and get feedback from Toolpad team.

Context

The core team is merging the CssVarsProvider and extendTheme into ThemeProvider and createTheme to prevent the confusion of using the APIs. The detail is in this issue.

Changes

  • Remove the CssVarsProvider because now ThemeProvider can handle with/without colorSchemes node.
  • I see that Toolpad has a custom logic to receive a theme with { light: Theme, dark: Theme } structure so this is untouched.
  • The dualTheme can be checked by reading { allColorSchemes } = useColorScheme() if allColorSchemes.length > 1.

For more detail of the implementation, see the Material UI PR: mui/material-ui#43115


  • I've read and followed the contributing guide on how to create great pull requests.
  • I've updated the relevant documentation for any new or updated feature.
  • I've linked relevant GitHub issue with "Closes #".
  • I've added a visual demonstration in the form of a screenshot or video.

@apedroferreira
Copy link
Member

Having allColorSchemes seems good to me if we plan to support any number of color schemes with any names.

Since CSS vars themes are becoming standard I wouldn't even support an object with { light: Theme, dark: Theme } in Toolpad anymore. Users would need to use a CSS vars theme if they wanted dual theming / a theme switcher and this way they would never be surprised by running into theme flash issues when server-side rendering, for example. I guess if we did this the implementation would be pretty straightforward?

@siriwatknp
Copy link
Member Author

Since CSS vars themes are becoming standard I wouldn't even support an object with { light: Theme, dark: Theme } in Toolpad anymore

Yes, if that's the case the AppThemeProvider can be removed. It will be much simpler.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 2, 2024
@siriwatknp siriwatknp added the proof of concept Studying and/or experimenting with a to be validated approach label Aug 6, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 6, 2024
@Janpot
Copy link
Member

Janpot commented Aug 6, 2024

@siriwatknp I updated the implementation a bit to make sure useColorScheme is never called when there is a v5 theme.

Verified that the implementation works against v5

@Janpot Janpot marked this pull request as ready for review August 8, 2024 10:09
@Janpot Janpot requested a review from a team August 8, 2024 10:10
@@ -60,8 +60,8 @@
"prop-types": "15.8.1"
},
"devDependencies": {
"@mui/icons-material": "6.0.0-beta.2",
"@mui/material": "6.0.0-beta.2",
"@mui/icons-material": "next",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything labeled as "next" will eventually get a stable v6 specifier

@Janpot Janpot changed the title [POC] Replace CssVarsProvider with ThemeProvider [core] Replace CssVarsProvider with ThemeProvider Aug 8, 2024
@Janpot Janpot mentioned this pull request Aug 8, 2024
1 task
@Janpot Janpot merged commit 1c4b88e into master Aug 8, 2024
13 checks passed
@Janpot Janpot deleted the new-theme-provider branch August 8, 2024 10:33
@Janpot Janpot mentioned this pull request Aug 8, 2024
6 tasks
Copy link
Member

@apedroferreira apedroferreira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great, thanks!

const demoTheme = extendTheme({
const demoTheme = createTheme({
cssVariables: {
colorSchemeSelector: 'data-toolpad-color-scheme',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this selector needs to be added every time I guess we could override it ourselves, or make it fully configurable. As right now I guess the AppProvider theming wouldn't work with any other selector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proof of concept Studying and/or experimenting with a to be validated approach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants