[system] Support callback value in styleOverrides slot

[siriwatknp]

This is the first PR as described in #30412

close #27415

📦 Code sandbox
🔗 Deploy preview

Summary

• add unit tests
• add types spec

Review suggestion
types packages/mui-material/src/styles/overrides.d.ts

---

• I have followed (at least) the PR section of the contributing guide.

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 43956fe

[siriwatknp]

add default type so it does not break the people who import ComponentsOverrides in whatever cases they are using.

[mnajdova]

Looks good generally. When we should deprecate the keys that we don't plan to support in v6?

[siriwatknp]

Looks good generally. When we should deprecate the keys that we don't plan to support in v6?

The deprecation will touch a lot of files (basically, all components) and the Codemod should be in place, so I propose we do the deprecation after we have migrated to the new URLs.

[mui-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 2122360

[siriwatknp]

@michaldudak tag you as another reviewer. As discussed with @mnajdova, we plan to mention ownerState on the docs so that developers can use it to conditionally apply styles. Are you good with this?

[siriwatknp]

moved Default props up

[siriwatknp]

@mnajdova I found some bug when doing this in Chip:

{
  MuiChip: {
    styleOverrides: {
      icon: ({ ownerState }) => ({ ... }), // this does not work
    }
  }
}

Some keys does not work because they are nested classNames:

// in Chip.js
const ChipRoot = styled('div', {
  name: 'MuiChip',
  slot: 'Root',
  overridesResolver: (props, styles) => {
    return [
      { [`& .${chipClasses.icon}`]: styles.icon }, // < function does not work here.
      // ...other styles
    ]
  }

To fix this, I added statement to check if it is a function:

// in Chip.js
const ChipRoot = styled('div', {
  name: 'MuiChip',
  slot: 'Root',
  overridesResolver: (props, styles) => {
    return [
      { [`& .${chipClasses.icon}`]: typeof styles.icon === 'function' ? styles.icon(props) : styles.icon },
      // ...other styles
    ]
  }

From what I check, there are not a lot of components that has nested className styles, so I think this is enough for the styleOverrides callback to work for non-slot keys. Also, they will be removed in the next major version anyway.

If you are okay with this approach, I will merge the PR.

[mnajdova]

@siriwatknp how about we resolve the keys of the styled before this. Won't something like this work?

index 42ae0c4cf4..a5c04ff9b3 100644
--- a/packages/mui-system/src/createStyled.js
+++ b/packages/mui-system/src/createStyled.js
@@ -133,9 +133,12 @@ export default function createStyled(input = {}) {
         expressionsWithDefaultTheme.push((props) => {
           const theme = isEmpty(props.theme) ? defaultTheme : props.theme;
           const styleOverrides = getStyleOverrides(componentName, theme);
-
+          const resolvedStyleOverrides = Object.keys(styleOverrides).reduce((acc, key) => {
+            acc[key] = typeof styleOverrides[key] === 'function' ? styleOverrides[key](props) : styleOverrides[key];
+            return acc;
+          }, {})
           if (styleOverrides) {
-            return overridesResolver(props, styleOverrides);
+            return overridesResolver(props, resolvedStyleOverrides);
           }

           return null;

[siriwatknp]

how about we resolve the keys of the styled before this. Won't something like this work?

Yeah, this is better.

[mnajdova]

@siriwatknp did you check that this would work with string templates?

[michaldudak]

@michaldudak tag you as another reviewer. As discussed with @mnajdova, we plan to mention ownerState on the docs so that developers can use it to conditionally apply styles. Are you good with this?

Sure, let them know about it. It's an external API after all.

Good job overall with the PR!

[siriwatknp]

did you check that this would work with string templates?

Yes, a test added in packages/mui-system/src/createStyled.test.js

[oliviertassinari]

I landed here after seeing the notion of deprecation in #30668.

We might be jumping one step head too quickly on this one. I will add a new comment on why we added the "variant" API in the first place, it's true purpose doesn't seem covered in #30412 or in #28107 (from my perspective on why we added it in the first place, it wasn't because of JSS, when we added it we were already adding emotion from what I recall, we knew this PR was going to be possible).

Edit: done #30412 (comment)

[siriwatknp]

Okay, I will remove the deprecation section for now and update the #30412.

[genepaul]

This appears to have been merged with the deprecation warning of the variants prop: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/styles/components.d.ts#L42

But the comments above lead me to believe that it wasn't supposed to be deprecated, or at least there was some contention on that point? At the very least, the documentation says nothing about the deprecation of this prop: https://mui.com/customization/theme-components/#adding-new-component-variants

This is leading to a very confusing experience. Should I or should I not use variants? Is styleOverrides really the replacement? I agree with @oliviertassinari that perhaps this was done prematurely.

[oliviertassinari]

@genepaul I think that the main learning here is that when MUI adds deprecations or breaking changes, it's best to do it in isolated PRs so that the matters are isolated and communicated very clearly to the community. I would propose this as a best practice.

Should I or should I not use variants?

Regarding the topic specifically, I don't know 😁 @siriwatknp should be able to provide lights on this. What I had understood is that this PR adds great new functionality. However, this new API duplicates to some extent with the variants, so we have to consider what is the best practice API, what serves developers the best in the long term.

[genepaul]

@oliviertassinari I get that, but this PR added a deprecation notice for variants, when whether or not that should be deprecated appears to be in dispute. Or am I misunderstanding?

[siriwatknp]

@oliviertassinari I get that, but this PR added a deprecation notice for variants, when whether or not that should be deprecated appears to be in dispute. Or am I misunderstanding?

Thanks for noticing this and your feedback is very welcome. To give you some context, this PR is the result of #30412. At first, I thought that using callback already covers the variants, but later @oliviertassinari has a different thought so we don't have a conclusion about deprecation yet.

To summarize:

• variants is not deprecated.
• I will remove the deprecation mentioned in other issues as well as this PR.
• We will make sure to improve the deprecation process in the future

[genepaul]

@siriwatknp - thank you for the clarification. That was my main confusion on all of this - I love the efforts to consolidate the many ways to do component theming, I was just surprised when I went to do something the documentation told me I could do, and the code told me it was deprecated. Just wanted to make sure it was clear. I appreciate all the work on this library!