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

Extending the outer theme with a nested ThemeProvider throws an error if css variables are active #44429

Open
mattstobbs opened this issue Nov 16, 2024 · 6 comments
Assignees
Labels
customization: theme Centered around the theming features docs Improvements or additions to the documentation

Comments

@mattstobbs
Copy link

mattstobbs commented Nov 16, 2024

Steps to reproduce

Steps:

  1. Open this link to live example: (required) https://codesandbox.io/p/sandbox/exciting-rhodes-zk9kwk
  2. Enable css variables in the outer ThemeProvider
  3. Nest a ThemeProvider within the outer ThemeProvider
  4. Attempt to extend the outer theme following the guidance in Theming (the codesandbox is adapted straight from that example)
  5. When createTheme is called with the outer theme spread into it, an error is thrown. This seems to be because the outer theme has a vars field, which is not allowed as an input to createTheme.

Current behavior

The following error is thrown:

MUI: `vars` is a private field used for CSS variables support.
Please use another name.

Expected behavior

An error should not be thrown and the outer theme should be able to be extended even if CSS variables are enabled.

Context

We're trying to extend the outer theme with additional styles, as described in the docs.

I'm happy to raise a PR to address this issue, once a fix has been agreed.

Your environment

npx @mui/envinfo
  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: Extending theme

@mattstobbs mattstobbs added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Nov 16, 2024
@zannager zannager added the customization: theme Centered around the theming features label Nov 18, 2024
@siriwatknp
Copy link
Member

siriwatknp commented Nov 18, 2024

The goal of having CSS variables is to have a single instance of createTheme for your app. If you want to customize some part of the app with different values, you can override some tokens to a specific scope:

<ThemeProvider theme={theme}>

  ….
  
      <div style={{ '--mui-palette-primary-main':  }}></div>
      
      
</ThemeProvider>

create nested themes would create a huge size of stylesheet with duplicate tokens.

However, if you really want to do it, you can get away with this:

<ThemeProvider
  theme={(theme: Theme) => {
    const { vars, …rest } = theme;
    return createTheme({
      ...rest,
      palette: {
        ...rest.palette,
        primary: {
          main: green[500],
        },
      },
    })
  }}
>
  <Checkbox defaultChecked />
  <Checkbox defaultChecked color="secondary" />
</ThemeProvider>

@siriwatknp siriwatknp added status: waiting for author Issue with insufficient information and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 18, 2024
@mattstobbs
Copy link
Author

@siriwatknp Thanks for your reply!

Your response answers the case in the attached sandbox. For additional context for the situation we're in, we're not looking to override the tokens within the inner theme provider, we mostly override the components (e.g. set the default variant for text fields to "outlined"). Destructuring vars from the theme works well in that case though.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Nov 18, 2024
@DiegoAndai DiegoAndai moved this to Backlog in Material UI Nov 21, 2024
@aarongarciah
Copy link
Member

@mattstobbs yeah, you can override component props in nested theme providers as shown in this demo: https://codesandbox.io/p/sandbox/clever-ives-f42vn3?workspaceId=d6fa470f-2079-42a1-aa65-469b49bbde6c

@siriwatknp I wonder if we could improve the logic to avoid throwing an error. I'd expect to be able to spread one theme object into another. I wonder if ignoring vars when passed to createTheme and throwing a warning instead of an error could be better.

@siriwatknp
Copy link
Member

siriwatknp commented Nov 25, 2024

@aarongarciah In this case for @mattstobbs, it's better to use DefaultPropsProvider directly. My initial proposal is not good because the components inside the nested ThemeProvider will no longer use CSS variables.

https://codesandbox.io/p/sandbox/peaceful-bhabha-l2m634?file=%2Fsrc%2FDemo.tsx%3A32%2C17

What we missed is docs about DefaultPropsProvider and nested ThemeProvider and its use cases/warnings so that people are aware of the tradeoffs (not related to this issue though).

@siriwatknp siriwatknp added support: question Community support but can be turned into an improvement and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Nov 25, 2024
@github-project-automation github-project-automation bot moved this from Backlog to Done in Material UI Nov 25, 2024
@siriwatknp siriwatknp reopened this Nov 25, 2024
Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

@mattstobbs How did we do? Your experience with our support team matters to us. If you have a moment, please share your thoughts in this short Support Satisfaction survey.

@siriwatknp siriwatknp added docs Improvements or additions to the documentation and removed support: question Community support but can be turned into an improvement labels Nov 25, 2024
@mnajdova
Copy link
Member

mnajdova commented Jan 8, 2025

We should have an API for nested CSS variables injection. We can detect if a nested ThemeProvider is used, and only add the CSS vars that are overriden. We shouldn't recommend to people to use different API if they want nested themes in my opinion.

@mnajdova mnajdova added this to the Material UI v7 (draft) milestone Jan 8, 2025
@DiegoAndai DiegoAndai moved this from Done to Backlog in Material UI Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
customization: theme Centered around the theming features docs Improvements or additions to the documentation
Projects
Status: Backlog
Development

No branches or pull requests

5 participants