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

Problem with the latest MUIv5 and classes/withStyles overrides of nested elements #62

Closed
synaptiko opened this issue Feb 12, 2022 · 8 comments

Comments

@synaptiko
Copy link
Contributor

synaptiko commented Feb 12, 2022

Hello,

when I created this PR #54 we were still using MUIv4. We are using overrides with classes prop and withStyles of MUI components a lot and since we started migrating to MUIv5, I've noticed something strange.

I've create a demo here:

I followed the recommended steps from both tss-react and MUI projects to eliminate issues with the setup.

The problem in a nutshell is the following, when you have a component with sub-elements and you'd like to override default styles (let's say color or background color), one would expect to use what was possible in MUIv4:

const StyledChip = withStyles(Chip, (theme) => ({
  root: {
    backgroundColor: theme.palette.primary.main,
    color: theme.palette.primary.contrastText,
  },
  deleteIcon: {
    color: theme.palette.primary.contrastText,
    '&:hover': {
      color: `${theme.palette.primary.contrastText}6`,
    },
  },
}));

But it doesn't work because of CSS selector specificity:
image

The only solution I found is with:

const StyledChip = withStyles(Chip, (theme, _props, classes) => ({
  root: {
    backgroundColor: theme.palette.primary.main,
    color: theme.palette.primary.contrastText,
    [`& .${classes.deleteIcon}`]: {
      color: theme.palette.primary.contrastText,
      '&:hover': {
        color: `${theme.palette.primary.contrastText}6`,
      },
    },
  },
}));

Unfortunately it requires a lot of refactoring in our code and it's also quite hard to reason about, you need to know that you are fighting with specificity here (which I assumed that emotion-based solution should help to avoid).

I'm not 100% sure if it's a problem in tss-react itself or if it's more about how MUIv5 handles classes props.

@synaptiko
Copy link
Contributor Author

I checked MUI's issues as well and the only partially related thing I found is mui/material-ui#30727

But it's about "state" rules, while in my case I'm overriding sub-component (slot).

I also took a look on the implementation of Chip to see how classes "resolution" is implemented but I got lost in it. But I can see now that they internally generate styles with specificity 2 for the sub-components: https://github.com/mui/material-ui/blob/master/packages/mui-material/src/Chip/Chip.js#L139

But then I'm not sure why classes is exposed at all, if it only can add new style props. Unless I'm missing something important here 🙂

@garronej
Copy link
Owner

garronej commented Feb 12, 2022

Hi @synaptiko,

I might be able to do something about it but I am not sure.

Is the problem that the collor in the root take priority over the color in deleteIcon?

image

PS: Maybe related: #38

@synaptiko
Copy link
Contributor Author

The issue is specifically about sub-component. Even if color from root is removed, the problem persist. And it's not only about Chip, I had similar issue with a few other components.

Thank you, I'll try to get help from MUI.

@garronej
Copy link
Owner

garronej commented Feb 12, 2022

Is it that (1) doesn’t take precedence over (2) ?

  deleteIcon: {
    color: theme.palette.primary.contrastText, // (2)
    '&:hover': {
      color: `${theme.palette.primary.contrastText}6`, // (1)
    },
  },

Or is it that neither (1) nor (2) take precedence over an internal MUI style that defines color?

@synaptiko
Copy link
Contributor Author

@garronej It's about specificity of what's generated by withStyles/makesStyles vs what's used by MUI internally for deleteIcon. I've added :hover only as an additional thing one could override but it's not specific to it. Even without :hover the color is not correct as it can be seen here: https://muiv5-problem-with-classes.vercel.app/

Here are the generated styles:
image

image

What's coming from MUI is .mui-hke119 .MuiChip-deleteIcon and what's coming from makeStyles is .tss-wtj64x-deleteIcon. But I would expect classes will merge both under .mui-hke119 .MuiChip-deleteIcon.

@garronej
Copy link
Owner

garronej commented Feb 12, 2022

OK, thank you for the extra details.
I was making sure there wasn't anything I can do for you and I am afraid this is effectively the case.

In defense of the MUI team, if they are facing all theses specificity issues it is because they are allowing users to use styled-component instead of @emotion as underlying style engine. ref. It makes their job 10 times harder, I am not sure it's worth it.

@synaptiko
Copy link
Contributor Author

Got it, I understand it's not an easy job to support customization, especially with more than one CSS engine. In any case, thanks for you help, feel free to close this issue and let's see if someone from MUI can help or if we just need to refactor the code to increase specificity.

@garronej
Copy link
Owner

I'm interested to have the follow up on this.
And feel free to come back if you think there is something I can do from TSS-react.

gitbook-com bot pushed a commit that referenced this issue Mar 8, 2022
gitbook-com bot pushed a commit that referenced this issue Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants