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

[RFC] Consolidate component theming into a one API (aka styleOverrides) #30412

Closed
siriwatknp opened this issue Dec 27, 2021 · 16 comments · Fixed by #42614
Closed

[RFC] Consolidate component theming into a one API (aka styleOverrides) #30412

siriwatknp opened this issue Dec 27, 2021 · 16 comments · Fixed by #42614
Labels
breaking change discussion package: system Specific to @mui/system RFC Request For Comments v6.x

Comments

@siriwatknp
Copy link
Member

siriwatknp commented Dec 27, 2021

As discussed in #28107 (comment), there are 2 current APIs that provide component theming.

- styleOverrides
- variants

A quick history of why there are 2 APIs: In v4, styleOverrides is not able to support dynamic props via callback due to the limitation of JSS. That's when we introduce another API (variants) to inject a specified style when a set of props is matched. However, it causes some confusion with the component variant prop (ex. #29455) and exposes new syntax but has limitations because it cannot create matching props like this (variant === 'filled' && color !== 'success').

If we step back, we will see that both of the APIs are trying to provide the same thing, component theming. Fortunately, in MUI v5, the styleOverrides supports callback thanks to emotion & styled-components so it is worth to drop variants and expose only one theming API.

For history, @oliviertassinari explain in #30412 (comment)

Here are all the requirements that styleOverrides can/should support:

  1. static style that target slots
    {
      MuiButton: {
        styleOverrides: {
          root: {
            background: 'linear-gradient(...)'
          },
          startIcon: {
            marginRight: 12,
          }
        }
      }
    }
  2. [new] dynamic style from props (in v4, we provide classes that can be used to target each prop)
    {
      MuiButton: {
        styleOverrides: {
          // colorPrimary: { ... }, in v4 we do this
          // which can be translated into
          root: (props) => ({
            ...props.color === 'primary' && { ... }
          }),
          // other slots can be function as well
        }
      }
    }
  3. [new] support array syntax for using a lot of pseudo-class (or nested class), so that it does not override each other. (this is an existing feature that emotion & styled-components are already supported)
    {
      MuiButton: {
        styleOverrides: {
          // root: props => ({
          //  '&:after: { display: 'block', color: 'red' },
          //  ...props.color === 'primary' && {
          //    '&:after: { color: 'blue' }, // this `display` in the upper :after is gone because this is object.
          //  }
          // }),
          root: [
            { '&:after: { display: 'block', color: 'red' } },
            props => ({
              ...props.color === 'primary' && {
                '&:after: { color: 'blue' }, // this `display` in the upper :after is still there.
              }
            })
          ]
        }
      }
    }
  4. [new] it can be used with the new theme.unstable_sx utility!
    {
      MuiButton: {
        styleOverrides: {
          root: ({ theme }) => theme.unstable_sx({
            p: 2,
            bgcolor: 'primary.main',
          }),
        }
      }
    }

Deprecation

  • styleOverrides should contain only slot keys and deprecate all classes because those can be done by using callback. eg: for Button, the styleOverrides should be
    MuiButton: {
      styleOverrides: {
        root: {},
        startIcon: {},
        endIcon: {},
        // outlinedPrimary, outlinedSecondary, ..., sizeSmall, sizeLarge, ...etc. should be deprecated but still work in v5
      }
    }
  • variants should be deprecated. (We don't plan to deprecate variants at this moment)

Plan

I think it is possible to aim for NO breaking change, but some deprecations are expected. We can try to create codemod that helps migrate the deprecation.

1st PR #30524

  • make styleOverrides support callback (only for slot keys)
  • deprecate variants themeing feature reverted
  • update docs about the callback support

2nd PR

  • deprecate non-slot class keys (only in theming styleOverrides)
  • create codemod for
    • migrate classes format in styleOverrides to callback
    • migrate variants to styleOverrides callback (We don't plan to deprecate variants at this moment)

3rd PR

  • [optional] a blog post announcement

In the end, we can close #22259, #27415 and #28107

cc @mui-org/core

@mnajdova
Copy link
Member

Agree with the API.

If we update the types for styleOverrides to support the functions and arrays, we could immediately unlock this API. After this, we could deprecate variants and the other class keys in favor of using the root and other slots keys in the styleOverrides.

Then, in v6 we can drop the other keys and provide codemod for each key in the components (this is the most important part). We want to make v6 easy for adoption after making that many breaking changes in v5.

@danilo-leal
Copy link
Contributor

Just because I was playing with the API intended to be deprecated the other day and for me to learn as well: if I only want to override styles of the small size variant, how would I do that, given that sizeSmall would be deprecated?

@siriwatknp
Copy link
Member Author

Just because I was playing with the API intended to be deprecated the other day and for me to learn as well: if I only want to override styles of the small size variant, how would I do that, given that sizeSmall would be deprecated?

You can look up the props via a callback.

{
  MuiButton: {
    styleOverrides: {
      root: (props) => ({
        ...props.size === 'small' && { ... }
      }),
    }
  }
}

@danilo-leal
Copy link
Contributor

Got it. Thanks, Jun! Must say though that just from a first glance perspective, it looks more complex than using the sizeSmall approach 😅

@abhic91
Copy link

abhic91 commented Dec 29, 2021

Hi, I didn't want to create a new issue without knowing if I am doing something wrong. The styleOverrides does not work.

@mnajdova

https://codesandbox.io/s/globalthemeoverride-material-demo-forked-srvfc?file=/demo.tsx
https://stackoverflow.com/questions/69577570/mui-v5-styleoverrides-not-working-with-themes

Am I doing something wrong?

@siriwatknp
Copy link
Member Author

siriwatknp commented Jan 4, 2022

Got it. Thanks, Jun! Must say though that just from a first glance perspective, it looks more complex than using the sizeSmall approach 😅

One reason might be the familiarity of the existing API (sizeSmall). It might be frustrating for some people at the beginning
because they have to change the way they used to do but I am pretty sure that we will have fewer issues in long term. Also, I believe that some people will love it (at least I am 😂 because I can use sx in the theme!).

sizeSmall approach is a NO-go for sure as far as I know. Take a look at chipClasses, there are around 20 classes that are incomplete and we can't create every possible combination of classes because we don't know who will use or not use them. So it is definitely better to let developers own the styling via callback.

I also wonder once we deprecated all the classes, how much bundle size will decrease.

@danilo-leal
Copy link
Contributor

Right, thanks for the explanation! I believe it's a great idea to add this reasoning to the documentation when explaining the changes :)

@mhespenh
Copy link

mhespenh commented Jan 19, 2022

Hi, just a very happy MUI user with a quick clarifying question on this set of changes so we can plan our migration:

Is the ButtonPropsVariantOverrides interface going away as well, or will we still be able to define custom variants and style them using the new styleOverrides callback?

For example, will this continue to work (works now, in v5.3.0):

declare module '@mui/material/Button' {
  interface ButtonPropsVariantOverrides {
    customVariant: true;
  }
}

const theme = createTheme({
  MuiButton: {
    styleOverrides: {
      root: ({ ownerState }) => ({
        ...(ownerState.variant === 'customVariant' && { ... })
      }),
    }
  }
});

@siriwatknp
Copy link
Member Author

Is the ButtonPropsVariantOverrides interface going away as well, or will we still be able to define custom variants and style them using the new styleOverrides callback?

No worries, the ButtonPropsVariantOverrides interface still works as-is. Once you augment the interface, you will be able to see it in the ownerState as well! That's one of the main goals of having style overrides as callback!.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 21, 2022

A quick history of why there are 2 APIs

@siriwatknp From my perspective, this is not the reality of what happened. It was not about JSS. The introduction of the variant API: #15573, #21749. But I also realize that these issues don't cover the point I'm going to detail: static definition. #21648 might be the most insightful on the history, with the different options that we considered.

From my perspective, the key selling point of the variant API is that the resolution is static. In practice, the variant API allows:

  1. Empower low-code design tools to build a design system in a Figma-like interface. This is the path that Modulz is pushing, and why they have a variant API in stitches https://stitches.dev/docs/variants
  2. Empower smarter TypeScript resolution, with no efforts: [system] Support new props with variants #19466 (comment)
  3. It might empower static extraction, or at least caching to improve performance: https://vanilla-extract.style/documentation/recipes-api/.
  4. It might force an API on the developers to more clearly organize their styles, hopefully making it easier to maintain, when the styleOverrides is for more niche use cases of customizations.

Deprecation: styleOverrides should contain only slot keys and deprecate all classes because those can be done by using callback. eg: for Button, the styleOverrides should be

👍

I think that it could be an issue on its own as we already talked about doing it somewhere, explaining in detail the pain and the gain. I believe the why is about:

  • removing a foot gun. It only works in x% of the cases and is hard to guess what each prop does. The API docs cover it a bit better, but still, it's not as clear as using props or the ownerState.
  • reducing bundle size

variants should be deprecated.

It's not obvious to me that it should be. One option could be to frame styleOverrides and variants for two distinct use cases, encouraging the latter, and using the former as an escape hatch.

@oliviertassinari
Copy link
Member

@o-alexandrov
Copy link

o-alexandrov commented May 9, 2022

@oliviertassinari You mentioned vanilla-extract, does it mean that at some point in the future, MUI is considering to let developers to replace built-into mui emotion with plain CSS?

To be transparent, while asking this, I don't think the replacement is even possible not mentioning the effort, but I'm just in hopes MUI team considers to have styles without a runtime (plain css for better performance).

  • vanilla-extract seems to result in best performance, while keeping satisfactory DX

@mnajdova
Copy link
Member

You mentioned vanilla-extract, does it mean that at some point in the future, MUI is considering to let developers to replace built-into mui emotion with plain CSS?

To be transparent, while asking this, I don't think the replacement is even possible not mentioning the effort, but I'm just in hopes MUI team considers to have styles without a runtime (plain css for better performance).

vanilla-extract seems to result in best performance, while keeping satisfactory DX

We initially envisioned being able to create a wrapper for any CSS in JS solution, like the ones we have for emotion & styled components, @mui/styled-engine and @mui/styled-engine-sc. At MUI, we don't plan at this moment adding support for more CSS in JS solutions, but if the community wants to give a try by creating a new standalone packages for some other styling solutions, it would be interesting for us to see if the abstractions work :)

@siriwatknp siriwatknp added this to the v6 milestone Aug 2, 2022
@oliviertassinari oliviertassinari added the RFC Request For Comments label Aug 21, 2022
@oliviertassinari
Copy link
Member

As I understand it, moving forward with this plan would solve #38544 for free, and help a bit with performance. Each styled component wouldn't need to resolve all the theme.components.NAME.styleOverrides values.

@delijah
Copy link

delijah commented Dec 2, 2023

Would it make sense to extend the variants theming feature towards a more similar approach as styleOverrides ? To me it feels like the variants theming feature is missing a lot .

It would be awesome to have something like that:

// ...
MuiCard: {
  variants: [
    {
      props: { variant: 'newOne' },
      styleOverrides: {
        // ...
      }
  ]
}
// ...

What is also a bit confusing to me is the fact, that we do not get the scoped theme in the current variants style function. Therefore we have to use something like that:

// ...
MuiCard: {
  variants: [
    {
      props: { variant: 'newOne' },
      style: ({ theme }) => {
        return {
          color: theme[THEME_ID].palette.secondary.main,
        };
      }
  ]
}
// ...

@siriwatknp
Copy link
Member Author

I would like to open the discussion again since we added variants to styled API to support both emotion and zero runtime.

This makes the theme.variants unnecessary because users can specify variants inside each slot's styleOverrides. The benefits I see are:

  • Consistent API. What users write with styled is the same as styleOverrides which lets the code to be moved between instance and theme.
  • Solve the slot issue [RFC] Extend variants to support slots #28107. The current theme.variants only supports the root slot (need to increase CSS specificity to override a slot which does not work well for theming components) but theme.styleOverrides can be targeted at any slot.
  • Since styled argument and theme.styleOverrides are the same, the code should be simpler and the types should be the same.

cc @mnajdova @brijeshb42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change discussion package: system Specific to @mui/system RFC Request For Comments v6.x
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

9 participants