Class precedence #17

janus-reith · 2021-08-24T14:49:12Z

Context:

MUI v5 beta 4
Nextjs
Using makeStyles with tss-react

I'm used to passing classes to MUI components in a classes object in order to overwrite styles.
However, the Material UI default classes always seems to be appended later, causing the custom styles to be ignored.

For example:

const useStyles = makeStyles()((theme: Theme) => ({
  myButtonRoot: {
    backgroundColor: "green",
  },
}));

...

const { classes } = useStyles();

<Button color="primary" classes={{ root: classes.myButtonRoot  }}  />;

The resulting css will list that green background, but it will be overwritten by the own MUI Button styling, since that has a background of the primary theme color.

The custom makeStyles classes should come after/have priority over the default ones of that component.

The text was updated successfully, but these errors were encountered:

janus-reith · 2021-08-24T15:08:58Z

I tried adding a custom cache just for the makeStyles Wrapper:

const { makeStyles } = createMakeStyles({
  useTheme,
  cache: createCache({ key: "my-prefix-key" })
});

While the global default one just keeps the key "css" and is set up like defined in the MUIv5 nextjs example.

That way, the custom classes there actually have priority over the MUI ones however it still breaks the precedence of a className over the other classes.

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Edit: So I just verified, that only comes up when using a different cache. Using the same cache, I have the original error of classes not taking priority over MUI ones, but the className works as expected and also correctly has priority over the MUI classes.

garronej · 2021-08-24T15:55:51Z

Sorry if I am missing the point, I haven't read your message in detail. I will do if the following code do not solve your problem:

makeStyles.ts

export const cache= createCache({ key: "my-prefix-key" })

const { makeStyles } = createMakeStyles({
  useTheme,
  cache
});

page/document.tsx

import { createDocument } from "tss-react/nextJs";
import { getCache } from "tss-react/cache";
import { cache } from "../shared/makeStyles";

const { Document } = createDocument({ "caches": [ getCache(),  cache ] });


export default Document;

pages/index.tsx

import { getCache } from "tss-react/cache";

export default function Home() {
    return (
        <>
            <Head>
                //...
            </Head>
            <CacheProvider value={getCache()}>
                <MuiThemeProvider theme={theme}>
                    <CssBaseline />
                    <Root />
                </MuiThemeProvider>
            </CacheProvider>,
        </>
    );
}

janus-reith · 2021-08-24T16:05:26Z

Hey @garronej, thank you for the reply, yes that is basically what I'm doing with a few exceptions:

I dont create the cache in my makeStyles wrapper
_document.tsx has a custom getInitialProps in my case, however I think I wrapped things properly in a way createDocument would too
I dont use a the Cache provider or ThemeProvider per page, but have it in _app.js. Also, I don't use getCache there, but directly import the same shared createEmotionCache function. Just switched to getCache to be sure, but it doesn't make a difference.

I will try to create a minimal example on codesandbox to demonstrate the issue.
I will use the examples you gave above, since I expect the issue to be the same.

garronej · 2021-08-24T16:12:28Z

Thank you, sorry it didn't do it for you.

You can start from the test app located at src/test/ssr in this repo.

you can run it by doing:

git clone https://github.com/garronej/tss-react
cd tss-react
yarn
yarn build
yarn start_ssr

You can fork, modify the example in order to show me what isn't working as expected.

janus-reith · 2021-08-24T16:37:52Z

Oh, Im seeing this a bit too late, I already crated a quick example here:
https://stackblitz.com/edit/nextjs-4wchjr (Stackblitz since Codesandbox decides to act crazy again)

garronej · 2021-08-24T16:48:59Z

Ok thanks for taking the time to create a sandbox,

I see that:

It isn't working as expected with multiple caches.
It isn't working as expected when we use the mui classes props.

I have to fix that!

For now this is the best workaround I can offer: https://stackblitz.com/edit/nextjs-5bjvvn?file=src%2Fcomponents%2FsomeButton.tsx

garronej · 2021-08-24T16:50:16Z

If you feel you can do it, PR are welcome.

janus-reith · 2021-08-24T16:54:20Z

Thanks for the quick response!
Simply switching to className sadly is not option, since in my projects classes is used to customize various classes of a MUI component.

Happy to submit a PR once I find out how to control the order of styles here :)

janus-reith · 2021-08-24T17:14:00Z

I just started wondering, since Material UI internally uses @emotion/styled instead of @emotion/react, if that somehow would take priority.

import { makeStyles } from "../shared/makeStyles";
import styled from '@emotion/styled'

const useStyles = makeStyles()(() => ({
    otherText: {
        color: "orange"
    }
}));

const Basic = ({ className }) => {
    const { classes } = useStyles();

    return (
        <div className={`${className} ${classes.otherText}`}>Some text</div>
    )
}

const SomeStyled = styled(Basic)`
  color: hotpink;
`

However, this doesn't seem to be the case, since the text would properly render in orange,.
So since that got me curious, I just tried a static classname:

<Button
    classes={{
        root: "SOMESTATICNAME"
    }}
>
    text
</Button>

And realized that indeed SOMESTATICNAME would be at the start of the classes in the resulting element, not at the end.
So this problem seems to be about Material UI v5 in general, and not specific to this library, since the same issues would occur using static css.

Feel free to close this, or maybe keep it open since this would be the first starting point for users coming from v4 and expecting to be able to continue styling MUI components the same way they did before. I will search for any mention of that change in behavior in the MUI repo.

janus-reith · 2021-08-24T17:45:39Z

~~Submitted a new issue about this here: mui/material-ui#27945~~

Update: I got on the wrong track here, order of css classes was not the issue.
Now If don't get things wrong, it would boil down to this:

The emotion styles I define in some component would need to come AFTER the ones generated from the MUI component rendered INSIDE that component in the created CSS if I'm not mistaken.

Although, simply using a separate emotion cache and making all classes there come after the MUI ones / have priority over them does not seem to be the full solution either, since as described above in #17 (comment) that could still break order, with a custom classname not being prioritized over these classes.

garronej · 2021-08-26T01:26:44Z

Ok I thee where where a lot of back and forth. Sorry I wasn't able to to go through tss-react issues the past few days. I will have a carfull look at this as soon as I can.

garronej · 2021-08-29T17:12:32Z

Ok I think it's solved, let me publish a fix

garronej · 2021-08-29T22:02:59Z

Hi @janus-reith,
I think everything is fixed now, I didn't change anything in the code but I updated the instruction on how to set things up. See the updated readme.

Do not esitate to reopen if you still have issues but it have been carefully tested

garronej · 2021-08-29T22:04:29Z

See the diff of the README to know how to update your configuration to make things work as expected. Thank you for reporting.

garronej · 2021-08-30T10:31:28Z

I, however, opened another issue on the material-ui repo following up yours.

garronej · 2021-08-31T20:01:15Z

Hi @janus-reith,
Just a small notice to inform you that I updated the instructions on how to setup tss with mui yet again and I also made some small api changes.
Sorry for the instability of the API. Everything will be freezed once mui release.
See new instructions

janus-reith · 2021-09-01T08:53:19Z

Hey @garronej, thanks, yes it seems to be the solution that I also found in the meantime and described above.
#17 (comment)

That one issue however still remains unsolved if I'm not mistaken:

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Edit: So I just verified, that only comes up when using a different cache. Using the same cache, I have the original error of classes not taking priority over MUI ones, but the className works as expected and also correctly has priority over the MUI classes.

Since emotion will derive the priority of styles based on where in the three they are defined, that still doesn't work.
Here is a simple example:
My component, lets call it MyCustomButton, is a wrapper around the MUI Button and passed some classes, like a root one that defines that the Button by default is "green".
That custom component MyCustomButton just passes down all other props to the Button.

Now, I have some outer component that actually uses this Button, lets call it MyCustomPage.
MyCustomPage will make use of MyCustomButton, but instead of the default green, wants it to be blue.
So it defines a class with styles for a blue Button, and passes it: <MyCustomButton className={classes.blueButton}.

In v4 that would work fine.
But now, the resulting button will still be green - A class definition with styles for the original green button comes later, since first the Outer styles of MyCustomPage are defined, afterwards the inner ones of MyCustomButton.

While easy to work around this in a new project, this can make things quite hard with an existing codebase and is a dealbreaker for a simple transition from v4 to v5 if that pattern was used before.

One workaround that can help: On the outer class defined in MyCustomPage, add "&&" so it gets more priority:

myClassName: {
 "&&": {
      backgroundColor: "blue"
   }
}

Looks a bit weird, but probably better than using !important on each declaration.
It would end up in the resulting css as myClassName.myClassName

In a bigger project, the workflow for this can be like this:

Spot all Components that use MUI components and pass classes
Check each of them if they also add className, if yes add "&&".
Check each use of these components that wrap MUI Components, within order Components, if they pass props, maybe including className, and also add "&&" there.

This could cover usual usecases, but certainly not all of them. There could be situations where you have composition of multiple levels of nested styles, this could probably be covered using "&&&", "&&&&" and so on, you get the idea.

garronej · 2021-09-01T22:41:23Z

Tanks for rising this concern.

this can make things quite hard with an existing codebase and is a dealbreaker for a simple transition from v4 to v5 if that pattern was used before.

I agree, this is a problem. We want to make the migration as smooth as possible.

Maybe if material-ui devs address this issue we could, by moving back to using a unique cache, make it work like it used to but I don't see any other way from where I stand.

What I would recommend doing is using tss's cx function that allow to give priority of one class over an other:

export type Props = {
    className?: string;
    children: ReactNode;
};

export function MyButton(props: Props){

    const { className, children } = props;
   
    const { classes, cx }= useStyles();

    return (
        <MuiButton classes={{
            ...classes,
            "root": cx(classes.root, classeName)
        }}>
            {children}
        <MuiButton>
    );
    
}

I don't know how common is the pattern you describe. Maybe I should add some instruction in the readme about that.
What do you think ?

garronej · 2021-09-01T22:50:48Z

Proof

janus-reith · 2021-09-02T12:43:08Z

Maybe if material-ui devs address this issue we could, by moving back to using a unique cache, make it work like it used to but I don't see any other way from where I stand.

Yes, thats the same impression I got.

What I would recommend doing is using tss's cx function that allow to give priority of one class over an other

Interesting, I wasn't aware of that. I wonder if it could be wrapped in some clever way, to avoid needing to do this per component, but so far none comes to mind.

I don't know how common is the pattern you describe. Maybe I should add some instruction in the readme about that.
What do you think ?

That could be helpful - While I'm not sure how many people do it like that usually, passing a className as a prop doesn't seem to uncommon.

garronej · 2021-09-05T15:55:01Z

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Hi @janus-reith, correcty me if I am wrong but this doesn't seem to be actuate. sandbox

janus-reith · 2021-09-05T17:10:26Z

Before in MUIv4 with makeStyles:
I could pass a root class for a Button, e.g. containing background: "red", within classes.
Now if I also pass a className to that Button, with the class also being created using makeStyles, which e.g. has background: "blue", that one would have priority over what is defined within classes.

Hi @janus-reith, correcty me if I am wrong but this doesn't seem to be actuate. sandbox

Hey @garronej , I can't really make sense of that sandbox, it only shows one h1 and two h2 each receiving a class built with makeStyles? 🤔 It also doesn't run since there is an import for a non-existent styles.css - Did codesandbox by chance mess something up here?

garronej · 2021-09-05T17:28:37Z

Sorry I didn't save: https://codesandbox.io/s/makestyles-forked-txfry?file=/src/index.js

janus-reith · 2021-09-05T17:54:47Z

Sorry I didn't save: https://codesandbox.io/s/makestyles-forked-txfry?file=/src/index.js

Ah, I just read your issue in the MUI repo too and figured you were referring to the same thing.
I gave more details on the issue further in this comment: #17 (comment)
Rereading my statement above about classnames and classes, it might've sounded a bit misleading what my point is there, it was about how these style definitions relate, not about how classes and className relate in general.
In my example, the passed className is in the upper scope, and is passed to a Button in a component down the tree - That one has classes.root with a custom color set, and the outer className is able to overwrite it.

I took your Sandbox and added another Button that demonstrates the issue:
https://codesandbox.io/s/makestyles-forked-run7q

As far as I understand things, you won't be able to recreate the same logic with MUI v5 and tss-react currently - The definiton of the cyan inside the button comes later, so that one is used instead of the pink background from className

garronej · 2021-09-05T18:51:33Z

The example you gave, behave exactly the same with mui v5 and tss-react proof.

And if I move the definition of WrappedButton to the index and make it use the same useStyles the button behave like the others:
your sandborx modified

janus-reith · 2021-09-05T20:21:42Z

And if I move the definition of WrappedButton to the index and make it use the same useStyles the button behave like the others:
your sandborx modified

Sure, that's expected

The example you gave, behave exactly the same with mui v5 and tss-react proof.

This is confusing, I'm struggling to puzzle the pieces together, since with my earlier testings, this did not work properly. I think this was due to the two separate Caches being necessary, is that not the case anymore? I might need to follow up more on the changes in the latest release then.

garronej · 2021-09-05T20:34:40Z

In the latest release of tss-react the contextual cache provided by <CacheProvider /> is no longer picked up. By default tss-react uses import { getTssDefaultEmotionCache } from "tss-react".
If you want tss to use a specific cache there is a provider only for tss <TssCacheProvider />.
So yes in the latest latest version two caches are still required. It won't be the case if the mui team decides to address this but it doesn't seem to go that way.

janus-reith mentioned this issue Aug 25, 2021

composeClasses should append keys from classes, not prepend them before the result of getUtilityClass mui/material-ui#27945

Closed

2 tasks

garronej added a commit that referenced this issue Aug 29, 2021

Repoduce the error #17

fe62662

garronej added a commit that referenced this issue Aug 29, 2021

Enable classes prop to work in mui v5 #20 #19 #18 #17 #15

465be5f

garronej added a commit that referenced this issue Aug 29, 2021

Repoduce the error #17

561e804

garronej added a commit that referenced this issue Aug 29, 2021

Enable classes prop to work in mui v5 #20 #19 #18 #17 #15

89ddc75

garronej closed this as completed Aug 29, 2021

garronej added a commit that referenced this issue Aug 29, 2021

Enable classes prop to work in mui v5 #20 #19 #18 #17 #15

8c24db0

garronej added a commit that referenced this issue Sep 1, 2021

Add test for cx giving priorities #17 (changelog ignore)

3ea50b3

garronej mentioned this issue Sep 4, 2021

classes styles don't override default when using @emotion/react mui/material-ui#28045

Closed

2 tasks

gitbook-com bot pushed a commit that referenced this issue Jan 23, 2022

GitBook: [#17] No subject

73ce2f5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Class precedence #17

Class precedence #17

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 24, 2021 •

edited

Loading

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 24, 2021 •

edited

Loading

janus-reith commented Aug 24, 2021

garronej commented Aug 24, 2021

garronej commented Aug 24, 2021

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 26, 2021

garronej commented Aug 29, 2021

garronej commented Aug 29, 2021

garronej commented Aug 29, 2021 •

edited

Loading

garronej commented Aug 30, 2021

garronej commented Aug 31, 2021

janus-reith commented Sep 1, 2021

garronej commented Sep 1, 2021 •

edited

Loading

garronej commented Sep 1, 2021 •

edited

Loading

janus-reith commented Sep 2, 2021

garronej commented Sep 5, 2021 •

edited

Loading

janus-reith commented Sep 5, 2021

garronej commented Sep 5, 2021

janus-reith commented Sep 5, 2021 •

edited

Loading

garronej commented Sep 5, 2021 •

edited

Loading

janus-reith commented Sep 5, 2021

garronej commented Sep 5, 2021 •

edited

Loading

Class precedence #17

Class precedence #17

Comments

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021 • edited Loading

garronej commented Aug 24, 2021 • edited Loading

janus-reith commented Aug 24, 2021 • edited Loading

garronej commented Aug 24, 2021 • edited Loading

janus-reith commented Aug 24, 2021

garronej commented Aug 24, 2021

garronej commented Aug 24, 2021

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021

janus-reith commented Aug 24, 2021 • edited Loading

garronej commented Aug 26, 2021

garronej commented Aug 29, 2021

garronej commented Aug 29, 2021

garronej commented Aug 29, 2021 • edited Loading

garronej commented Aug 30, 2021

garronej commented Aug 31, 2021

janus-reith commented Sep 1, 2021

garronej commented Sep 1, 2021 • edited Loading

garronej commented Sep 1, 2021 • edited Loading

janus-reith commented Sep 2, 2021

garronej commented Sep 5, 2021 • edited Loading

janus-reith commented Sep 5, 2021

garronej commented Sep 5, 2021

janus-reith commented Sep 5, 2021 • edited Loading

garronej commented Sep 5, 2021 • edited Loading

janus-reith commented Sep 5, 2021

garronej commented Sep 5, 2021 • edited Loading

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 24, 2021 •

edited

Loading

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 24, 2021 •

edited

Loading

janus-reith commented Aug 24, 2021 •

edited

Loading

garronej commented Aug 29, 2021 •

edited

Loading

garronej commented Sep 1, 2021 •

edited

Loading

garronej commented Sep 1, 2021 •

edited

Loading

garronej commented Sep 5, 2021 •

edited

Loading

janus-reith commented Sep 5, 2021 •

edited

Loading

garronej commented Sep 5, 2021 •

edited

Loading

garronej commented Sep 5, 2021 •

edited

Loading