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

[theme] Be able to use string templates inside createMuiTheme #18000

Closed
valentinbourqui opened this issue Oct 23, 2019 · 10 comments
Closed

[theme] Be able to use string templates inside createMuiTheme #18000

valentinbourqui opened this issue Oct 23, 2019 · 10 comments
Labels
new feature New feature or request

Comments

@valentinbourqui
Copy link

valentinbourqui commented Oct 23, 2019

Be able to use string templates when you use createMuiTheme and you want override a component like MuiButton :

..with jss-plugin-template plugin :

const theme = createMuiTheme({
  overrides: {
    MuiButton: {
      root: `
        font-size:  14px;
        color: ${props => props.theme.palette.text.primary};
      `,
    },
  },
}) 

++ Unify style with the same structure as I can do inside "makeStyles" or "withStyles".
++ Use global props defined inside theme

@eps1lon
Copy link
Member

eps1lon commented Oct 23, 2019

What you probably want is tagged template literals not just template literals. You would need to use an extra helper like

root: css`
       font-size:  14px;
       color: ${props => props.theme.palette.text.primary};
     `

and even then would we need to run the string through a CSS parser.

I don't think this is worth the effort but let's see what others think.

@eps1lon eps1lon added new feature New feature or request package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. labels Oct 23, 2019
@oliviertassinari
Copy link
Member

Agee, this will likely be possible with styled-components.

@oliviertassinari oliviertassinari added the priority: important This change can make a difference label Oct 24, 2019
@oliviertassinari oliviertassinari removed the priority: important This change can make a difference label Dec 1, 2019
@oliviertassinari oliviertassinari changed the title Be able to use string templates inside createMuiTheme [theme] Be able to use string templates inside createMuiTheme Nov 12, 2020
@oliviertassinari oliviertassinari removed the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Nov 12, 2020
@oliviertassinari
Copy link
Member

An update, this issue is being resolved in v5 thanks to #22342 and the new @material-ui/styled-engine package.

const theme = createMuiTheme({
  components: {
    MuiSlider: {
      styleOverrides: {
        root: css`
          background-color: red;
        `
      }
    }
  }
});

https://codesandbox.io/s/continuousslider-material-demo-forked-9595u?file=/demo.tsx

However, the usage of props isn't supported, so far, we aim to address the issue with the variants key of the theme.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 17, 2021

Closing for #15573. Developers can use the variant API to implement something similar.

@oliviertassinari
Copy link
Member

@mnajdova Actually, this makes me wonder. There is currently no way to dynamic render CSS in the theme based on the props. Is this something we need?

@mnajdova
Copy link
Member

@mnajdova Actually, this makes me wonder. There is currently no way to dynamic render CSS in the theme based on the props. Is this something we need?

We have this ability with the variants API as you've mentioned. As far as I remember from our discussions before, we still support the styleOverrides because we don't want to introduce too many breaking changes. The keys there are strict props combination, the variants API supports any props combination. Is there other use-case that the variants API doesn't support, or do you propose we change the API?

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 19, 2021

Interesting. A couple of thoughts:

  • If flat CSS specificity in the theme overrides is important, we will need to reorganize the styleOverrides and keep each slot accessible in the theme, in the long term.
  • There is currently no way to dynamic handle props from the theme. Not sure if it's important. For instance, if a developer wants to implement the paper box shadow elevation, he will have to create a component with the styled() API. The variants could work but it feels like a hack.

@mnajdova
Copy link
Member

Interesting. A couple of thoughts:

  • If flat CSS specificity in the theme overrides is important, we will need to reorganize the styleOverrides and keep each slot accessible in the theme, in the long term.
  • There is currently no way to dynamic handle props from the theme. Not sure if it's important. For instance, if a developer wants to implement the paper box shadow elevation, he will have to create a component with the styled() API. The variants could work but it feels like a hack.

I don't understand why the variants API is a hack. As far as I remember we had discussion around adding root in the variants API, but we decided intentionally to not add that complexity - #21648 (comment) (had to search for it 😄 ) In my opinion, flat CSS specificity is important when defining the initial styles (that is why we decided to go with styled() component for each slot, not by adding the all styles on the root with increasing specificity), but for overrides, I don't think it's that important as it is meant to be overriding the initial styles.

I am ok with restructuring that styleOverrides to match this if we don't like the variants API, but I think we decided to go with new API to avoid doing any breaking changes in the styleOverrides. Probably we can just add support for the variants API under styleOverrides as an addition, not breaking change, but I'll have to check if this is possible. In that case we should drop the variants API as it doesn't make sense to have both..

@mnajdova
Copy link
Member

One more thought, if we support props in the styleOverrides, I don't think it make sense to have keys like, colorPrimary, which will bring us again to the breaking changes.

@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 19, 2021

@mnajdova Sounds good, I think that it's a topic for v6 or v5.x. We can wait, see how things settle once v5 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants