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

[CSS form label issue] #103

Closed
jano-gazashvili-axon opened this issue Aug 24, 2022 · 39 comments
Closed

[CSS form label issue] #103

jano-gazashvili-axon opened this issue Aug 24, 2022 · 39 comments

Comments

@jano-gazashvili-axon
Copy link

After updating tss-react from v3.7.1 to v4.0.0, the Form labels are changed and lost CSS, they have covered text fields

<FormControl
      fullWidth={props.fullWidth}
      required={props.required}
    >
        <InputLabel
          error={props.error}
          htmlFor={props.htmlFor}
          required={props.required}
        >
          <Typography fontSize={14} fontWeight="400" component="span">
            {props.label}
          </Typography>
        </InputLabel>
        {textField}
</FormControl>
@garronej
Copy link
Owner

Hi @jano-gazashvili-axon,
Sorry about the update issue.
I see nothing related to TSS in the snipet you shared.
Please provide additional information or event better, a test repo.

Best

@jano-gazashvili-axon
Copy link
Author

jano-gazashvili-axon commented Aug 24, 2022

Hi @garronej, (before update/after update), i showed just @mui/material simple code which is responsible for text field and label, it works correctly before tss-react update, nothing to share directly about tss-react, only thing what i can say is that what you see on images

image

image

@garronej
Copy link
Owner

I don't question that there is a bug but it can't be narrowed down to the code you shared, TSS is simply not involved here.
Maybe you have a container that uses selectors that affects the component in question.
Maybe it's the update of MUI that is at fault.
Maybe the use of TSS global style...?

@jano-gazashvili-axon
Copy link
Author

I don't question that there is a bug but it can't be narrowed down to the code you shared, TSS is simply not involved here. Maybe you have a container that uses selectors that affects the component in question. Maybe it's the update of MUI that is at fault. Maybe the use of TSS global style...?

if i will update TSS version then input behavior is not relevant, when i roll back version input starts working correctly. i am talking when all packages up-to date expected tss-react

@garronej
Copy link
Owner

garronej commented Aug 25, 2022

@jano-gazashvili-axon I am very willing to do whatever it takes to solve your issue but you have to help me help you.
In the snippet of code you shared TSS is simply not involved.
What technology are you using? Next.js, Create React App?
The best would be for you to create a minimal repo that reproduces the issue or to give me access to your codebase.

If you can't do either of these, try giving me as much context and code snippet as possible so I can figure out what's wrong.

@jiby-aurum
Copy link
Collaborator

@garronej I noticed styles being lost once I updated to v4 as well, but only when I remove providing the cache, which is mentioned as not required here https://docs.tss-react.dev/update-to-v4#removing-noise.

So seemingly not providing the cache, still won't work okay with v4. I will try to figure out what is the actual code that affects when I have time, but for now I put back the cache stuff.

@jiby-aurum
Copy link
Collaborator

@garronej well, as far as I found, its styling unrelated to tss, so seemingly mui still needs the CacheProvider to style correctly

@garronej
Copy link
Owner

garronej commented Sep 2, 2022

@jiby-aurum,
Thank you for the feedback!
Do you confirm upgrading @mui/material to a version newer than 5.9.3?
Are you in a Next.js setup?

Anyway if you could tell me what are the problematic components....

@garronej
Copy link
Owner

garronej commented Sep 2, 2022

@garronej well, as far as I found, its styling unrelated to tss, so seemingly mui still needs the CacheProvider to style correctly

Oh noo,
If it's the case it mean that I need to dig into MUI's codebase once more.
Are tou using the latest version of @emotion/react and @emotion/styled ?

What options do you provide when creating the cache?

@jiby-aurum
Copy link
Collaborator

jiby-aurum commented Sep 2, 2022

@garronej yes, I already had 5.10.3, and no its not Nest.js setup, plain react project with mui and tss-react.

Component was InputAdornment, code below

<InputAdornment position="start" style={{ color: 'black' }}>
      <Icons.Search40 />
</InputAdornment>

it was not applying the style. See below the css .css-9kt8nu-Search-input seemingly was not added as style without the cache
Screenshot 2022-09-02 at 2 21 37 PM
Screenshot 2022-09-02 at 2 29 29 PM

@jiby-aurum
Copy link
Collaborator

@garronej yes I'm on 11.10.4 for @emotion/react and @emotion/styled

its plain, just like it is in the documentation
Screenshot 2022-09-02 at 2 34 20 PM

@garronej
Copy link
Owner

garronej commented Sep 2, 2022

Thank you for the detailed report,
Can you tell me if it works correctly when you set prepend to false?

@jiby-aurum
Copy link
Collaborator

jiby-aurum commented Sep 2, 2022

@garronej nope, does not work if prepend is false, same behaviour as not providing the cache

@jiby-aurum
Copy link
Collaborator

jiby-aurum commented Sep 2, 2022

@garronej well well well, its not that the styles are not inject, they are injected, with prepend false or without the cache, its injected after the normal styles so its not overridden
Screenshot 2022-09-02 at 2 46 04 PM

sorry did not notice it earlier, as I did not scroll down 🤭

@garronej
Copy link
Owner

garronej commented Sep 2, 2022

@jiby-aurum,
Yes yes, I figurred out but still. It should't matter.
Here, in the last screen shot you have two cache coexisting in your app (mui and css). This isn't normal.
When you explicidely provide a cache MUI is supposed to pick it up. Are you sure, in your project you and MUI are using the same instance of @emotion/react it wouldn't suprise me if you had both:
node_modules/@mui/material/node_modules/@emotion/react and node_modules/@emotion/react
Is it the case?

And when you provide no cache could you give me the same screen shot where I can see everything?

@jiby-aurum
Copy link
Collaborator

@garronej I think multiple versions of @emotion/react problem is there in my project, as recently I started to see this console warning You are loading @emotion/react when it is already loaded. Running multiple instances may cause problems. This can happen if multiple versions are used, or if multiple builds of the same version are used.

However, I checked my node_modules and @emotion/react is only at the top level, and only version 11.10.4

Find the styles without providing the cache, in this case, it seems to be just the css cache.
Screenshot 2022-09-03 at 5 12 18 PM

@garronej
Copy link
Owner

garronej commented Sep 3, 2022

Is Storybook involved?

@jiby-aurum
Copy link
Collaborator

@garronej nope, btw, I was quickly checking where the two calls for @emotion/react comes from, and one is from mui, and the second from tss-react

@jiby-aurum
Copy link
Collaborator

@garronej I will rollback my updates to investigate a bit where I started to get the duplicate multiple emotion running warning, because with mui 5.8.0 and css 3.6.2, it was not there

@jiby-aurum
Copy link
Collaborator

@garronej its really weird, I started to get the warning You are loading @emotion/react when it is already loaded. Running multiple instances may cause problems. This can happen if multiple versions are used, or if multiple builds of the same version are used. just after upgrading @emotion/react from 11.9.0 to 11.10.4, no other changes at all

@jiby-aurum
Copy link
Collaborator

jiby-aurum commented Sep 3, 2022

@garronej I have identified the issue, and it's the build system I am using which is vite. Seemingly with vite, in dev run, mui loads the es version of @emotion/react and tss-react the cjs version, which means its two different instances of @emotion/react, which leads to the problem.

@jiby-aurum
Copy link
Collaborator

@garronej which means the solution for this should be very simple as well, to ship the es version as well. I see that currently tss-react is only shipps as cjs. It would be just a build step as the project is anyways es

@jiby-aurum
Copy link
Collaborator

@garronej I did quickly try it out and it fixes the problem for me, I opened a quick PR. Would appreciate if you can push a new version soon.

@garronej
Copy link
Owner

garronej commented Sep 9, 2022

@jano-gazashvili-axon This shoud be fixed now with the latest release.
Please feel free to reopen if it isn't the case.

Thanks for reporting,

Best regards,

@garronej garronej closed this as completed Sep 9, 2022
@ivan-romanchuk-axon
Copy link

ivan-romanchuk-axon commented Sep 15, 2022

@garronej Hi! Still persists with version 4.1.3

With version 3.7.1 labels are OK, like on first screenshot of this comment

With version 4.1.3 labels are broken, same as on second screenshot of mentioned comment

@garronej
Copy link
Owner

Hi @ivan-romanchuk-axon,
Thanks for reporting.
Can you provide a minimal repo where I could reproduce the bug?

@garronej garronej reopened this Sep 15, 2022
@ivan-romanchuk-axon
Copy link

ivan-romanchuk-axon commented Sep 15, 2022

@garronej Sure! Here is a simple sandbox

@ivan-romanchuk-axon
Copy link

ivan-romanchuk-axon commented Sep 15, 2022

@garronej updated sandbox to reproduce an issue. Wrapped TextField with StyledEngineProvider. With version 3.7.1 label is OK, with version 4.1.3 => broken

@garronej
Copy link
Owner

@ivan-romanchuk-axon Sorry I didn't find the time to get to it yet but I will.

@garronej
Copy link
Owner

garronej commented Sep 23, 2022

@ivan-romanchuk-axon Ok I see, it's a bug on the MUI side.
I'll make a PR but in the meantime here is a workaround:

    <InputLabel
-      classes={{ root: classes.label, error: classes.error }}
+      classes={{ error: classes.error }}
+      className={classes.label}
    >

@garronej
Copy link
Owner

Hi @ivan-romanchuk-axon,
I have submited a pull request on MUI.

@garronej
Copy link
Owner

Hi @ivan-romanchuk-axon and @jano-gazashvili-axon this should be fixed in @mui/material 5.10.7.
Can you confirm?
Reopen if it's not the case!

Best regards,

@ivan-romanchuk-axon
Copy link

Hello @garronej!

Thanks for your feedback and commitment to this issues.

Sorry, I was moving out and that's why — busy. I will try to check ASAP, at least in codesandbox.

@garronej
Copy link
Owner

@ivan-romanchuk-axon No problem, you are wellcome, thank you for helping me figuring out what was wrong.

I checked in the sandbox. It does work :)

Best,

@ivan-romanchuk-axon
Copy link

ivan-romanchuk-axon commented Oct 4, 2022

Hi @ivan-romanchuk-axon, I have submited a pull request on MUI.

Hello @garronej!

I can confirm that labels are fixed. But it seems that in your PR for MUI Select component was missed. After all updates (MUI and TSS), styles for Select are broken.

@garronej
Copy link
Owner

garronej commented Oct 4, 2022

Hey @ivan-romanchuk-axon,
Thanks for reporting again.

But it seems that in your PR for MUI Select component was missed.

Is that so ? Damn it!
I didn't directly touch the <Select /> but maybe one of the connex component where afected.

image

Woul you prepare me a Sandbox or better yet a demo repo where I can reproduce?

Best regards.

@ivan-romanchuk-axon
Copy link

Hello @garronej!

I updated a sandbox for reproducing issue with Select components. Now I chose tss-react version 3.7.1, you can see all styles apply well. Then change a version to an actual one, and you will see that styles are broken

@garronej
Copy link
Owner

garronej commented Oct 7, 2022

Thank toi for taking the time to produce a reproduction path.

@garronej
Copy link
Owner

garronej commented Oct 8, 2022

Hi @ivan-romanchuk-axon,
I issued a new PR on MUI for adressing the <Select /> issue.

mui/material-ui#34659

Best regards,

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

4 participants