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

[CssVarsProvider] Add CssVarsProvider in @mui/material #31138

Merged
merged 56 commits into from
Mar 30, 2022

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented Feb 18, 2022

Preview - https://deploy-preview-31138--material-ui.netlify.app/experiments/material-ui/css-variables/

TODOs:

  • Figure out why for light theme the same CSS vars are added two times
  • Test out custom themes with multiple color schemes
  • What should we do with values depending on JS transformations? - resolved for the color transofrmations
  • Add tests
  • Revert the changes in the DemoSandboxed

@mui-bot
Copy link

mui-bot commented Feb 18, 2022

Details of bundle changes

@material-ui/core: parsed: +2.26% , gzip: +2.44%
@material-ui/lab: parsed: +0.16% , gzip: +0.09%

Generated by 🚫 dangerJS against fe370af

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2022
@danilo-leal danilo-leal added the proof of concept Studying and/or experimenting with a to be validated approach label Feb 18, 2022
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 18, 2022
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 22, 2022
Comment on lines +14 to +21
resolveTheme: (theme) => {
const newTheme = {
...theme,
typography: createTypography(theme.palette, theme.typography),
};

return newTheme;
},
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
resolveTheme: (theme) => {
const newTheme = {
...theme,
typography: createTypography(theme.palette, theme.typography),
};
return newTheme;
},

From what I checked in createTypography, there is no need to recalculate here. Developer can customize the typography via experimental_extendTheme.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we can, as the typography is being generated by the palette, which based on the colorScheme set on the CssVarsProvider is being calculated.

Copy link
Member

@siriwatknp siriwatknp Mar 17, 2022

Choose a reason for hiding this comment

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

Okay, let's keep it like this but I think we should be able to remove it in the future. In createTypography.js, I don't see any usage of the palette internally. With the CSS variables, I don't see a need to accept typography as a function because developers can use the palette via CSS variables.

extendTheme({
  typography: {
    h1: {
      color: 'var(--joy-palette-text-primary)',
    }
  }
})

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Left a few comments. I'm excited!

@siriwatknp siriwatknp changed the title [POC] Text createCssVarsProvider for @mui/material [POC] Test createCssVarsProvider for @mui/material Mar 17, 2022
@mnajdova
Copy link
Member Author

@siriwatknp @danilo-leal I've created this PR for adding documentation around the CSS variables - mnajdova#32
I thought it would be easier to be reviewed in isolation from the implementation, but if you think that's not the best way, I will merge it on top of this PR.
Here is the preview: https://mnajdova-material-ui-zrzlejic1-mnajdova.vercel.app/experimental-api/css-variables/

@siriwatknp
Copy link
Member

I suggest to merge the implementation first because I feel that the doc would require some time to be finished.

@danilo-leal
Copy link
Contributor

Yup, agree with that! Pretty cool experimentation here though, curious to see how we're going to evolve with this in Material UI.

@siriwatknp
Copy link
Member

Yup, agree with that! Pretty cool experimentation here though, curious to see how we're going to evolve with this in Material UI.

I think the next step could be asking the community to help us update each component to support CSS variables (similar to the v5 transition #24405). Write some announcement blog post, docs.

@mnajdova
Copy link
Member Author

Alright then, I will revert the changes in the DemoSandbox and merge this one

@mnajdova mnajdova changed the title [POC] Test createCssVarsProvider for @mui/material [CssVarsProvider] Add CssVarsProvider in @mui/material Mar 30, 2022
@mnajdova mnajdova added the package: material-ui Specific to @mui/material label Mar 30, 2022
@mnajdova mnajdova merged commit 119c881 into mui:master Mar 30, 2022
@singerbj
Copy link

Question on css theme variables for MUI - this issue notes the feature as experimental. Are there plans documented anywhere for the feature to become....not....experimental? :)

@siriwatknp
Copy link
Member

Question on css theme variables for MUI - this issue notes the feature as experimental. Are there plans documented anywhere for the feature to become....not....experimental? :)

We are in the process of stabilizing the API in #40225

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: material-ui Specific to @mui/material 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.

5 participants