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

withStyles #16

Closed
garronej opened this issue Aug 23, 2021 · 22 comments
Closed

withStyles #16

garronej opened this issue Aug 23, 2021 · 22 comments

Comments

@garronej
Copy link
Owner

garronej commented Aug 23, 2021

Could you implement withStyles in tss-react too? It's convenient when just want to overwrite the style of a Mui Component.

const StyledLinearProgress = withStyles(LinearProgress)({
    root: {},
    // other LinearProgressProps.classes properties...
})

Originally posted by @Jack-Works in mui/material-ui#26571 (comment)

garronej added a commit that referenced this issue Aug 23, 2021
@garronej garronej changed the title Our project has 100% migrated to tss-react and feeling good. withStyles Aug 23, 2021
@garronej
Copy link
Owner Author

Hi @Jack-Works,
I have started to implement it but it turned out not to be as trivial as I though it would be.

  1. I gess we want to levrage mui's classes prop but there's an issue that have been open about it that I should adress first.
  2. I can't implement the API as in mui because if the style object is provided first the type of props argument can't be inferred.

I will work on theses points, feel free to give feedback on the branch I just created.

@Jack-Works
Copy link

I can't implement the API as in mui because if the style object is provided first the type of props argument can't be inferred.

I'm only focused on extending the existing components with classes props. In this case it;s easy to infer the types. Can you give an example of what are you considering?

function createMakeStyles<Theme>(useTheme) {
    function withStyles<T>(Component: React.ComponentType<T>) {
        type Classes = T extends { classes?: Partial<Record<infer U, string>> } ? U : never
        return function appendStyles(style: StyleOf<Classes, T>) {}
    }
}

@garronej
Copy link
Owner Author

garronej commented Aug 23, 2021

This is where I am at so far. I will do an overload of withStyles to leverage the mui classes prop.
I think our withStyles can just be a function that takes two arguments, the component and the css rather than a higher-order function.
In mui they do first the css then the component but it can't be typesafe in this order.
You propose to provide the component first then the css. Which works type-wise but do you see a use case that would justify it to be a higher-order function rather than just a function with two parameters?

@Jack-Works
Copy link

You propose to provide the component first then the css. Which works type-wise but do you see a use case that would justify it to be a higher-order function rather than just a function with two parameters?

I guess it can be a single function. It just looks like a styled to me. styled(Component)(style)

@Jack-Works
Copy link

const Out = function (props: Props) {

You should use forwardRef to make sure ref is forwarded

@garronej
Copy link
Owner Author

Ok, now that I have addressed the problem related to the mui classes props I will be abe to work on this.

By the way, the only way I found to make tss-react work flawlessly with mui v5 is to make material-ui and tss-react use a different cache.

image

When the cache is explicitly provided to createMakeStyle the contextual cache is no longer picked up. Is this something that could be troublesome in your case?

@Jack-Works
Copy link

When the cache is explicitly provided to createMakeStyle the contextual cache is no longer picked up. Is this something that could be troublesome in your case?

The contextual cache is important in our use case. Is there some features that cannot work with contextual cache? (We're not using SSR)

@garronej
Copy link
Owner Author

garronej commented Aug 30, 2021

Mui v5 classes prop cannot work if tss-react and mui uses the same emotion cache. Or rather it work but the style don't have priority so most often than not it will be overwriten by mui's default style unless you put ! important.
This is due to an optimisation that MUI does internally. The only way I found to bypass de problem is to make tss-react use a different cache.
If it's a problem for you I can try to work this out with the mui's team.
I oppened an issue on MUI.

@Jack-Works
Copy link

This is due to an optimisation that MUI does internally. The only way I found to bypass de problem is to make tss-react use a different cache.

Hi, it's OK to have a different cache for MUI and tss-react for us, but the cache must come from the React Context. e.g. We can write this:

<MuiEmotionCacheProvider cache={cache1}>
    <TSSReactEmotionCacaheProvider cache={cache2}>
        {children}

@garronej
Copy link
Owner Author

This is a can do, IDK why I didn't think of it myself. Let me implement that.

garronej added a commit that referenced this issue Aug 31, 2021
@garronej
Copy link
Owner Author

@Jack-Works I implemented the cache provider you suggested.
I hope it fits your needs.

@Jack-Works
Copy link

@Jack-Works I implemented the cache provider you suggested.
I hope it fits your needs.

Yes, it works in our product. Thanks for your work!

@Jack-Works
Copy link

I have upgraded to the latest version in DimensionDev/Maskbook#4163

@CodeWithMichal
Copy link

Hi, any update regarding withStyles ?:)

@garronej
Copy link
Owner Author

not yet, but I hope I'll find the time to finalize it by Monday.

@garronej
Copy link
Owner Author

@LabuzzMichal, sorry. I have worked extensively on this this week end but it still needs some more work before being released.

@CodeWithMichal
Copy link

@garronej thank you for update!
Take your time, you already did excellent job with makeStyles :)
I believe once its done your library will gather much more attention since upgrade from MUI 4 to MUI 5 will be much easier

@garronej
Copy link
Owner Author

garronej commented Oct 3, 2021

Just a quick update to notify that I am working on this

@garronej
Copy link
Owner Author

garronej commented Oct 4, 2021

I pull it off eventually 🎉
It was extremely challenging to get the type inference right.
I will need an extra session to redact the documentation and to some more advanced testing.
Maybe it would be cool as well if it was possible to write:

const MyDivStyled= withStyle("div", { "root": { "color": "red" } });

Anyway it's available on the latest patch update. I will do a minor bump when everything is tested and documented.

@CodeWithMichal
Copy link

Great, thank you :)
Will check it tomorrow

@CodeWithMichal
Copy link

CodeWithMichal commented Oct 8, 2021

Hi @garronej,
withStyles works like a charm :)

The only difference I noticed comparing to MUI withStyles is lack of possibility to write combined styles like below

const MyButton = withStyles(Button, (theme) => ({
  root: {
    borderColor: [theme.palette.grey[700], "!important"],
  },
}));

However it works fine if I just add these values

const MyButton = withStyles(Button, (theme) => ({
  root: {
    borderColor: theme.palette.grey[700] + " !important",
  },
}));

@garronej
Copy link
Owner Author

@LabuzzMichal thank you for the feedback.

It's all documented and tested

gitbook-com bot pushed a commit that referenced this issue Jan 23, 2022
gitbook-com bot pushed a commit that referenced this issue Jul 10, 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

3 participants