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

no example(s) setup showing what is required (cache/theme/? provider) #63

Closed
Morriz opened this issue Feb 15, 2022 · 15 comments
Closed

Comments

@Morriz
Copy link

Morriz commented Feb 15, 2022

Hi, thanks for all the hard work first of all. But would it be much to ask for some more extensive examples as to what is expected for this to work? I expect this lib has to order every bit off css to guarantee specificity, but following the docs it is not made clear what the minimal requirements are to do so.

@garronej
Copy link
Owner

garronej commented Feb 15, 2022

Hi @Morriz,
There are three canonical setup app here: https://github.com/garronej/tss-react/tree/main/src/test/apps
You have the instruction about how you can run them here:
https://github.com/garronej/tss-react#development

If you follow the instructions it works without any known issues with MUI: https://docs.tss-react.dev/readme-1
You also have specific instructions for SSR: https://docs.tss-react.dev/ssr

If you want to use a custom cache there are specific instruction here but I recommend you to stick with the default setup.

Best regard

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

Hi, tnx for your quick reply. But is that cache needed in the first place? I don't read anything saying so. Thing is that I don't see any specificity ordering after following the instructions, sometimes loading theme styles before, and sometimes loading it after component styles

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

do I have to forcefully use cx in every component now? And pass className to it from the props? To force funneling and ordering? I hope not, and don't think that would be needed...

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

Btw, I already consistently use these exports:

export const {
  makeStyles,
  useStyles, // <- To use when you need css or cx but don't have custom classes
} = createMakeStyles({ useTheme: getTheme })

(where getTheme returns my customized theme)

@garronej
Copy link
Owner

garronej commented Feb 15, 2022

But is that cache needed in the first place?

Yes, it is, MUI is using emotion internally. We have to explicitly provide an emotion cache that uses prepend: true to MUI so TSS can use another cache.
This way we ensure that the styles defined with TSS are injected after and thus have a higher priority than the ones defined internally by MUI.

do I have to forcefully use cxin every component now?

It's recommended to use cx instead of clsx yes as it gives you greater control. See this explanation

And pass className to it from the props?

I am not sure that I understand what you mean. You don't have to use cx if you have just one className.
If you want to merge the internal classes and the one that might have been provided as props into a single classes object you can use this utility.

I can see you are frustrated, the upgrade from material-ui v4 to MUI v5 is not as smooth as you hoped it would be.
Things with TSS are not exactly as they used to be but you'll find that it's an upgrade all across the board.
I encourage you to put your expectation aside and follow the instructions provided in the documentation.

Best regard,

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

thanks for the in depth response. So the cache is needed, and I followed the instructions, but I still don't set the specificity enforced. Let me recap:

I have this setup:

    <Suspense fallback={<Loader />}>
      <CacheProvider value={muiCache}>
        <ThemeProvider theme={getTheme()}>
          ...
            <CssBaseline />
            <Helmet titleTemplate='%s | xx' defaultTitle='xx' />
            <Context.Provider
              value={{
                ...
              }}
            >
              <Router basename={env.CONTEXT_PATH || ''}>
                <Switch>
                  <Route path='/' component={Dashboard} exact />

And I use makeStyles from

export const {
  makeStyles,
  useStyles, // <- To use when you need css or cx but don't have custom classes
} = createMakeStyles({ useTheme: getTheme })

I am not using cx much, and assume it is mostly used to order the classnames that are given to it.

So, what could I still be missing here? I see only one Route is working correctly, but see no differences in my handling there.

@garronej
Copy link
Owner

garronej commented Feb 15, 2022

I see no error in the snippet of code you shared but I don't have the full picture.
To help you debug I offer you three options (ordered by preference):

  1. Create a sandbox with stackblitz or code sandbox where you reproduce the issue you are facing.
  2. Share your repo, or as least a dumbed-down version of your repo, privately or publicly.
  3. Share your package.json dependencies, your full TSS and MUI setup and an example of how you are using TSS. Explain what isn't working as expected.

Best regards

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

That is awesome feedback. I will add you to the repo, as it is not really private just not ready for publication.

@garronej garronej reopened this Feb 15, 2022
@garronej
Copy link
Owner

garronej commented Feb 15, 2022

I cloned your repo, I see master is still using @material-ui/core (v4) and I see no mention of tss-react.
I looked on the other branches to see if there was a upgrade_to_muiv5 but I didn't see anything like it.

Do you confirm that you are in the process of upgrading from @material-ui/core (v4) to @mui/material (v5)?
Or do you just want to use tss-react with @material-ui/core? It's also possible but the setup isn't the same.

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022 via email

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022 via email

@garronej
Copy link
Owner

garronej commented Feb 15, 2022

I can't say for sure because there are some private dependencies that I can't install but looking at your package-lock.json it looks like you might have two copies of @emotion/react cohabitating in the same project.
If it's the case it's easy to check. You just have to change the key like:

image

And look in your chrome development tool if you can see the foo-bar appear.

If you can't then we found the problem. If you know how to fix it great, if not I can share some tips.

While I am at it. You are using the default mui Theme so you don't need to export makeStyles from your src/theme.ts file. You can just import it like import { makeStyles } from "tss-react/mui";.

Also you don't need the type annotation for Theme you can change:

image

Last thing, you don't need a callback if you're not using the theme nor any params:

image

(I have red squigly lines because I can't npm install)

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

Thanks! I also found the culprit: I had to import the old mui 4 as peerdep for @rjsf, but forgot to make the Layout's use v5. That solved it.

As to your last remark about not passing theme or typing it: I know so much, and eslint will warn me about it. However, I didn't receive type hints in your abstraction first, so that is why I type theme like that. But now that I solved it they appear ;)

@Morriz
Copy link
Author

Morriz commented Feb 15, 2022

Only thing still not working correctly is global styles, but that is matter of minutiously following the component upgrade instructions I guess...

Again, thanks so much!

@garronej
Copy link
Owner

Great, I am glad your problem is (mostly) solved.

gitbook-com bot pushed a commit that referenced this issue Mar 8, 2022
gitbook-com bot pushed a commit that referenced this issue Nov 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

2 participants