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

createGenerateClassName #28550

Closed
sampathmo opened this issue Sep 22, 2021 · 20 comments · Fixed by #29051
Closed

createGenerateClassName #28550

sampathmo opened this issue Sep 22, 2021 · 20 comments · Fixed by #29051
Labels
new feature New feature or request package: system Specific to @mui/system status: waiting for author Issue with insufficient information v5.x migration

Comments

@sampathmo
Copy link

In Mui-v4, createGenerateClassName's seed option was prefixed to all material-ui components. But with mui-v5 seed doesn't prefix.

This is the current behavior in mui- v5
Screen Shot 2021-09-22 at 7 34 21 PM

This is the expected behavior. This works fine in v4
Screen Shot 2021-09-22 at 7 35 12 PM

@sampathmo sampathmo added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Sep 22, 2021
@siriwatknp
Copy link
Member

@mnajdova Do we provide that option?

@mnajdova
Copy link
Member

@mnajdova Do we provide that option?

We haven't considered this in v5. Although I want to mention that it was something provided by the @mui/styles package which is not used in the material design components anymore. We would need to incorporate this in the system package.

I think it's worth adding it long term, especially when we would need to support multiple design systems.

@mnajdova mnajdova added new feature New feature or request package: system Specific to @mui/system and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Sep 24, 2021
@mnajdova
Copy link
Member

@siriwatknp should we mention this in the migration guide?

@sampathmo
Copy link
Author

sampathmo commented Sep 24, 2021

@mnajdova & @siriwatknp This option has been very useful for our team. Because we have different microservice UI projects integrated into one big orchestrator project. This helps us avoid CSS class names collision and keep unique across other microservice projects which use Material UI.

Also, It's a good idea to mention the migration guide, so at least those who are using this will know there is a breaking change when updated to Material UI v5.

@sampathmo

This comment was marked as duplicate.

@ruslanvyaznikov

This comment was marked as duplicate.

@siriwatknp
Copy link
Member

siriwatknp commented Oct 7, 2021

It is not supported in v5 but I think it is worth to do it to preserve what v4 can do.

Currently, the global classNames generated in the components are from utility function generateUtilityClasses. One constrain that I think should take into account is that the buttonClasses imported from Button should be able to receive the prefix.

// prefix is setup somewhere at the root of app
import { styled } from '@mui/material/styles';
import Button, { buttonClasses } from '@mui/material/Button';

const StyledButton = styled(Button)`
  .${buttonClasses.root}: { // buttonClasses.root should be '$prefix-MuiButton-root'
    ...
  }
`

With this constrain, I think we can create a global config that can be setup before the App is called. then the generateUtilityClasses can hook into that config at build time.

// usually index.js
import { GlobalConfig } from '@mui/material';

GlobalConfig.setup({ classNamePrefix: 'myapp' })

ReactDOM.render(<App />, ...)

Here is my POC. siriwatknp@6e40bcc. The benefit of this approach is that the component does not need to know how classname is generated, that's why the change in my POC is very small. However, if we use other approach like context or theme, we will need to touch all of the existing components.

Screen Shot 2564-10-07 at 10 40 41

Note: this approach is meant to be setup at build time, not run time.

What's next?

  • wait for feedback from team and community
  • if this idea sounds good, work on the naming and expose the api as unstable_ otherwise find other alternative solution.

cc @mnajdova @michaldudak

@ruslanvyaznikov
Copy link

@siriwatknp Thanks a lot, 'cause we have the same situation as in the @smora-gh 's comment #28550 (comment)

@mnajdova
Copy link
Member

mnajdova commented Oct 12, 2021

@siriwatknp could you open a PR for the change? We could add it under unstable_. I've seen multiple asks for it already.


One thing to keep in mind is that we probably want to keep the global pseudo classes that we are adding under two names 'Mui-disabledandWhateverpefix-disabled` so that we don't break the current implementation. Anyway, let's discuss this on the PR once we have it.

@sampathmo
Copy link
Author

sampathmo commented Oct 27, 2021

@siriwatknp Thank you for the fix. But one issue I see with Box Component. Prefix is not added to Box Component. Attached screenshot below. Any work around you suggest?

Screen Shot 2021-10-27 at 4 18 37 PM

Just in case adding screenshot without Mui replace. Above was with replacing Mui with Foo
Screen Shot 2021-10-27 at 4 25 07 PM

@siriwatknp
Copy link
Member

Prefix is not added to Box Component

will open a PR to fix it.

@sampathmo
Copy link
Author

sampathmo commented Oct 29, 2021

@siriwatknp I see one issue with Grid Container and Grid Item components. CSS specificity is not passed to child. Can you pls help.

before adding the prefix.
Screen Shot 2021-10-29 at 8 52 24 AM
Screen Shot 2021-10-29 at 8 57 23 AM

after adding the prefix
Screen Shot 2021-10-29 at 8 54 24 AM
Screen Shot 2021-10-29 at 8 56 45 AM

Issue with ToggleButtonGroup same as above.
before adding prefix
Screen Shot 2021-10-29 at 9 02 48 AM
Screen Shot 2021-10-29 at 9 03 17 AM

after adding prefix
Screen Shot 2021-10-29 at 9 01 35 AM
Screen Shot 2021-10-29 at 9 02 12 AM

@siriwatknp
Copy link
Member

siriwatknp commented Oct 29, 2021

Please open an issue with reproduction in codesandbox, so that we can track and other people can help.

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 1, 2021

This option has been very useful for our team. Because we have different microservice UI projects integrated into one big orchestrator project. This helps us avoid CSS class names collision and keep unique across other microservice projects which use Material UI.

@smora-gh It sounds like you are looking for emotion-js/emotion#2210. With Material UI v5 and @mui/styled-engine more specifically the class names of the styles injected by MUI are no longer global so createGenerateClassName won't help to solve CSS collision issues. Instead, you need to use one emotion cache per subtree as in the emotion issue that I linked.

@sampathmo
Copy link
Author

@oliviertassinari we haven't upgraded to styled components. All our components are still using JSS for styling.

@siriwatknp I did add the unstable_ClassNameGenerator import at the root before any other mui imports. But I still see the issue with Grid & ToggleButtonGroup components.

@siriwatknp
Copy link
Member

@oliviertassinari we haven't upgraded to styled components. All our components are still using JSS for styling.

@siriwatknp I did add the unstable_ClassNameGenerator import at the root before any other mui imports. But I still see the issue with Grid & ToggleButtonGroup components.

Please open another issue with reproduction.

@BiancaArtola
Copy link

BiancaArtola commented Nov 19, 2021

Any news on this? I am using microsites and without createGenerateClassName the styles overlap with each other.

The unstable_ClassNameGenerator works for microsites, but not for Storybook libraries for example, and this causes a lot of overlapping styles on my code. Will you fix this at some moment? Or do I need to continue using material v4?

@siriwatknp
Copy link
Member

siriwatknp commented Nov 21, 2021

but not for Storybook libraries for example

Please open a new issue with reproduction.

@BiancaArtola
Copy link

@siriwatknp #29856

@oliviertassinari oliviertassinari added the status: waiting for author Issue with insufficient information label Nov 29, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 29, 2021

we haven't upgraded to styled components. All our components are still using JSS for styling.

@smora-gh Then you can use the same API as in v4, @mui/styles (JSS) didn't change on this regard.

I'm adding the status: incomplete label because the problem is not clearly described. I believe we miss a reproduction.

As a general rule, I don't think that it's because problem A was fixed with solution X on version v4 and that it no longer works that we need to reapply solution X in v5. Maybe there is already a solution to the problem with emotion that is different than with JSS. For instance, in the case of @smora-gh maybe it's that there are v4 and v5 components mounted on the same page, and the app is not configured as in https://mui.com/x/react-data-grid/migration-v4/#using-mui-core-v4-with-v5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New feature or request package: system Specific to @mui/system status: waiting for author Issue with insufficient information v5.x migration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants