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.

Sign up for GitHub

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

Jump to bottom

[core] Replace CssVarsProvider with ThemeProvider #3872

Merged
merged 17 commits into from
Aug 8, 2024
Merged

[core] Replace CssVarsProvider with ThemeProvider #3872

merged 17 commits into from
Aug 8, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Aug 1, 2024 •
edited
Loading

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.

@bharatkashyap bharatkashyap requested review from apedroferreira and removed request for bharatkashyap August 1, 2024 04:13
@siriwatknp siriwatknp mentioned this pull request Aug 1, 2024
Merged
1 task
@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 •
edited
Loading

@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
Janpot
Janpot reviewed Aug 8, 2024
View reviewed changes
packages/toolpad-core/package.json
@@ -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
Merged
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
Closed
6 tasks
apedroferreira
apedroferreira reviewed Aug 9, 2024
View reviewed changes
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!

docs/data/toolpad/core/components/app-provider/AppProviderBasic.js
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
Reviewers

@Janpot Janpot Janpot approved these changes

@apedroferreira apedroferreira apedroferreira left review comments

Assignees
No one assigned
Labels
proof of concept Studying and/or experimenting with a to be validated approach
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@siriwatknp @apedroferreira @Janpot