[base] Make CSS class prefixes consistent #33411
Conversation
michaldudak
added
core
|
@material-ui/core: parsed: +0.07% , gzip: +0.12% |
michaldudak
force-pushed
the
consistent-classes
branch
2 times, most recently
from
2fd50b8 to
06f6b49
Compare
| @@ -29,10 +29,10 @@ export interface InputUnstyledClasses { | |||
| export type InputUnstyledClassKey = keyof InputUnstyledClasses; | |||
|
|
|||
| export function getInputUnstyledUtilityClass(slot: string): string { | |||
| return generateUtilityClass('MuiInput', slot); | |||
| return generateUtilityClass('BaseInput', slot); | |||
There was a problem hiding this comment.
I would expect
| return generateUtilityClass('BaseInput', slot); | |
| return generateUtilityClass('MuiInputUnstyled', slot); |
here so I don't have to think about what's going to be the class name when I'm looking at my code (import { InputUnstyled } from "@mui/material"). Commented in #33260 (comment).
Also, notice the company name Mui prefix to avoid class name collisions.
There was a problem hiding this comment.
We've discussed it already in the past (last year, possibly?) and settled on BaseComponentName. If I remember correctly the arguments for Base* over *Unstyled were that:
- It's shorter (and people have been complaining about the size of the HTML generated by Material UI and how it's hard to extract meaningful information).
- When Base components are used to build a design system, they are not "unstyled" anymore. So it's weird to see
.ButtonUnstyled { color: green; }- it was brought up by @mnajdova.
As for the "Mui" prefix - I'm ok with adding it. It'll make the class names a bit longer, but if you think it's important to have the company name there, I won't oppose it.
There was a problem hiding this comment.
We've discussed it already in the past
I remember a notion page about changing the name of the npm package so that it would feel more natural to expose headless components and hooks from @mui/base compared to @mui/unstyled, I think that it was also about covering the use case of the package, more than being descriptive of the content of the npm package.
Maybe there is another discussion about this for the components not the npm package that I wasn't involved in.
Ok, but assuming we change the name of the components from Unstyled to Base (and find a way to fix the conflict with Base Material UI components, e.g. ButtonBase that is styled), I still don't think that the changes proposed in this PR work. I think that it should be all-in: replace unstyled to base, everywhere at once and expose a codemod to handle it. I think that developers will be confused to have Unstyled sometimes and Base some other times when using MUI Base.
There was a problem hiding this comment.
Renaming components from *Unstyled to Base* wasn't on the table right now (and I'm not convinced we should do this - I feel everyone agreed on *Unstyled). It's just about CSS classes.
See #31974 (comment) for the discussion I found (cc @mnajdova). I remember talking about it in one of the meetings also, but could not find anything in the notes.
There was a problem hiding this comment.
I feel like what we are missing is giving a bit of background on why the change is being done in the PR description.
From different discussions here and there and things that I could remember we had this:
Mateiral UI components are named as MuiComponentName. When we started with Joy we had the same pattern, until we realized that having the same classes in Material UI and Joy components can be problematic, because they are not unique. This is why we decided to prefix the Joy components with Joy instead of Mui. In the mean time, the unstyled components were following this so-called pattern, of using MuiComponentName (without the Unstyled suffix, because the CSS would feel weird if it looked like: .MuiSomethingUnstyled: { color: red } // it's not unstyled anymore). Now that the Joy components are changed, it looks like the pattern is : ProductComponentName, instead of CompanyComponentName, hence the changes proposed on this PR.
I would vote for keeping what we have in this PR going forward, but I agree that this should not be something rushed up and released without clear rationales.
In addition to this, I would keep both classes for couple of versions giving people time to migrate to the new classes (in case if they haven't used the componentNameClasses constants).
There was a problem hiding this comment.
I think that it would be awesome to take these dimensions #33260 (comment) and compare how each of the different options that we have performs. I'm slowing down the decision process a bit because I feel that we are touching "customization", one of the biggest challenges that developers have https://mui.com/blog/2021-developer-survey-results/#what-else-can-we-do-to-improve-mui-for-you and 2. the decision feels hardly reversible 3. it went a bit against the expectation I had, creating confusion, hence maybe it could against the intuition of existing users.
Starting a bit quickly. Here are the different options that seem to emerge. Guess what's the class name of the thumb slot:
A. CompanyComponentName
import { Slider as SliderMd, sliderClasses as sliderMdClasses } from '@mui/material';
import { Slider as SliderJoy, sliderClasses as sliderJoyClasses } from '@mui/joy';
import { SliderUnstyled, sliderUnstyledClasses } from '@mui/base';
<SliderMd /> // sliderMdClasses.thumb = 'MuiSlider-thumb'
<SliderJoy /> // sliderJoyClasses.thumb = 'MuiSlider-thumb'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'MuiSlider-thumb'My initial assumption was that we were following this. I like how it's always the same. Sharing Mui between different products which might help us communicate that Joy and Material should be seen as themes for business users and that Material UI !== MUI. Otherwise, changing the Mui prefix to Md could be also a great way to communicate the latter point but not the former point. For developers that want to apply global CSS to customize the components => we would have to tell them: don't, it's a foot gun, would they do it, yeah maybe they would anyway.
B. Current tradeoff
import { Slider as SliderMd, sliderClasses as sliderMdClasses } from '@mui/material';
import { Slider as SliderJoy, sliderClasses as sliderJoyClasses } from '@mui/joy';
import { SliderUnstyled, sliderUnstyledClasses } from '@mui/base';
<SliderMd /> // sliderMDClasses.thumb = 'MuiSlider-thumb'
<SliderJoy /> // sliderJoyClasses.thumb = 'JoySlider-thumb'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'MuiSlider-thumb'Is not fully followed in HEAD, which we could scope down #33260 to
C. ProductComponentName
import { Slider as SliderMd, sliderClasses as sliderMdClasses } from '@mui/material';
import { Slider as SliderJoy, sliderClasses as sliderJoyClasses } from '@mui/joy';
import { SliderUnstyled, sliderUnstyledClasses } from '@mui/base';
<SliderMd /> // sliderMdClasses.thumb = 'MdSlider-thumb'
<SliderJoy /> // sliderJoyClasses.thumb = 'JoySlider-thumb'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'BaseSlider-thumb'or
import { Slider as SliderMui, sliderClasses as sliderMuiClasses } from '@mui/material';
import { Slider as SliderJui, sliderClasses as sliderJuiClasses } from '@mui/joy';
import { SliderUnstyled, sliderUnstyledClasses } from '@mui/base';
<SliderMui /> // sliderMuiClasses.thumb = 'MuiSlider-thumb'
<SliderJui /> // sliderJuiClasses.thumb = 'JuiSlider-thumb'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'BaseSlider-thumb'sliderUnstyledClasses.thumb = 'BaseSlider-thumb' feels surprising, my first thought was, wait, why?
D. CompanyModuleExportName
import { Slider as SliderMd, sliderClasses as sliderMdClasses } from '@mui/material';
import { Slider as SliderJoy, sliderClasses as sliderJoyClasses } from '@mui/joy';
import { SliderUnstyled, sliderUnstyledClasses } from '@mui/base';
<SliderMd /> // sliderMdClasses.thumb = 'MuiSlider-thumb'
<SliderJoy /> // sliderJoyClasses.thumb = 'MuiSlider-thumb'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'MuiSliderUnstyled-thumb'I like how it feels predictable, but I don't like how some of the unstyled components are re-exported as is from Joy UI or Material UI and hence would require us to find a way to customize the class names, seems too much overhead to implement in production to stay clear & predictable.
What's best? It's off my hands at this point, I have tried to share all of my thoughts on this, and I trust we will pick something that maximizes "simplicity".
There was a problem hiding this comment.
A. CompanyComponentName
For developers that want to apply global CSS to customize the components => we would have to tell them: don't
Why would you like to limit this way of customization? If someone prefers this way (as it's likely the easiest and doesn't require any styling libraries or preprocessors), I would let them.
B. Current tradeoff
To me option B is a no-go - why would Material and Base have the same prefix and Joy another one? It would confuse people.
C. ProductComponentName
Base" is longer to type and feels a bit less unique to avoid conflicts.
We can discuss the prefixes for each product. I don't insist on the prefix for MUI Base components to be Base
D. CompanyModuleExportName
This solution creates the longest class names.
I don't like how some of the unstyled components are re-exported as is from Joy UI or Material UI and hence would require us to find a way to customize the class names
It's a problem present in C as well.
There was a problem hiding this comment.
Of all the options here, I like A the best for the sake of simplicity. I also really like it from a branding angle, reinforcing MUI as a brand vs its products, while also showcasing the ways in which they are comparable and to some extent interchangeable.
The only thing that feels a bit incongruous to me is the fact that we use On second thought I guess it doesn't necessarily break the pattern. It's just maybe not obvious that Unstyled = Base.Unstyled when referring to Base components—if I'm following the pattern of sliderMdClasses and sliderJoyClasses then I would expect to see SliderBase and sliderBaseClasses rather than sliderUnstyledClasses. In other words, there's an extra step required to understand that Unstyled = Base. It's minor, but it feels like it breaks the pattern.
There was a problem hiding this comment.
Why would you like to limit this way of customization?
@michaldudak I think that the reasoning goes the other way around: What do we gain by dropping this support? Are there more pros than cons?
B is a no-go
I agree it's weird, and a convention harder to learn for developers.
D. It's a problem present in C as well.
I don't see this problem in D, maybe only because I have renamed the exports to avoid JavaScript variable conflicts in the example? In practice, developers won't do it like this, maybe I should haven't have renamed the exports after import 😁
There was a problem hiding this comment.
We are moving the discussion back to #33260.
This is what caught my attention to have a closer look. This sounds like we solve an inconsistency state issue by taking a path that doesn't minimize breaking changes to reach the consistent state 😁.
Do we have a GitHub issue about this one? I can think of one possible use case (I miss the context to think of others): for a company that wants to change
|
Could you explain what you mean by that? We don't introduce breaking changes in stable packages (nor do we intend to do so). From a point of view of Material UI users, nothing should change.
It could be used to avoid conflicts when using a library that uses MUI products internally (see #30925) or when styling different parts of the application differently (#28550).
I don't think so, I haven't created one myself yet. |
I understand #33260 as about solving the inconsistency in the convention used inside MUI Base. What seems to solve the inconsistency while minimizing the number of breaking changes seems to be about doing the change that I have suggested. I feel that in this PR, we trying to kill two birds with one stone, but the second bird (unstyled to base) doesn't seem to have its issue to explain the rationale of the breaking change, and is done in a way that leads developers to have an inconsistent experience. I would be in favor of:
With Material UI v4 I assume. I believe the problem is solved with Material UI v5 with the us of emotion, class names with styles are no longer global. With MUI Base, I guess the problem will happen, but how would we change the class names of only the components we care about? |
michaldudak
requested review from
siriwatknp and
danilo-leal
as code owners
github-actions
bot
added
PR: out-of-date
|
I've changed the approach based on the results of the poll. All components, regardless of the package they belong to, will use the Technically this is done by each package providing its own The |
Same as Autocomplete and other components that have nested components. I wonder if this should be fixed in this PR or a follow-up one.
|
|
Good catch. I'll see what I can do about it. You're right that it comes from components that use nested components in their roots (like Button using ButtonBase). |
I propose we leave it as is for now and see if people complain. Otherwise we may hurt perf for removing classes from string on each render. |
I think that it could be even better to start without the When you inspect the DOM nodes to find what I need to customize, it adds more "weight":
As far as I understand it, the people that these classes will benefit are people who don't follow best practices. We might not want to degrade the experience for the people who do. |
Make sense. |
github-actions
bot
added
the
PR: out-of-date
@oliviertassinari according to the poll results, if I understand you correctly, what you're proposing is the third most popular option with just 14% votes. Doesn't it defeat the purpose of the poll? |
|
@oliviertassinari, @siriwatknp, there's another option we can consider - we don't add the product classes ( |
That sounds good to me as well. I like the opt-in version so that the classes are included when they are needed. |
@michaldudak Correct, I'm challenging whether we are solving an existing problem with an extra class name to be able to differentiate the different default CSS/HTML structures.
So the problem considered would be: I have Material UI in my app, and now I would like to migrate to Joy UI. However, because I have done global CSS customizations on Material UI. So when I add the first Joy UI components the styles conflict. I guess we have a limited set of options to solve this:
From my perspective, the problem considered is one we shouldn't optimize for, so I would vote for the options in this order: |
|
We discussed this with @oliviertassinari in a meeting. We agreed to use this PR just to make the classes in Base consistent ( |
github-actions
bot
removed
the
PR: out-of-date
|
The PR has been updated to include the latest agreements. @oliviertassinari, @siriwatknp, it's ready to take another look on, if you'd like. |
| export { unstable_generateUtilityClass as default } from '@mui/utils'; | ||
| export type { GlobalStateSlot } from '@mui/utils'; |
There was a problem hiding this comment.
Why do you reexport these? Shouldn't all the components use class name utility from @mui/utils?
There was a problem hiding this comment.
I made it so each product could override the logic of generateUtilityClass, adding its own product class. Now, as we agreed not to use product classes, for the time being, this may not be strictly necessary
| import { unstable_generateUtilityClasses as generateUtilityClasses } from '@mui/utils'; | ||
| import generateUtilityClass from '../generateUtilityClass'; |
There was a problem hiding this comment.
| import { unstable_generateUtilityClasses as generateUtilityClasses } from '@mui/utils'; | |
| import generateUtilityClass from '../generateUtilityClass'; | |
| import { unstable_generateUtilityClass as generateUtilityClass, unstable_generateUtilityClasses as generateUtilityClasses } from '@mui/utils'; |
| import { | ||
| unstable_generateUtilityClass as generateUtilityClass, | ||
| unstable_generateUtilityClasses as generateUtilityClasses, | ||
| } from '@mui/utils'; | ||
|
|
There was a problem hiding this comment.
Off-topic
I don't fully remember: what's the long-term goal with@mui/utils and how do we decide which modules it owns? From what I understand, this package is used by all the others, it's a shared kitchen sink. I'm asking about it because it feels like many of the modules it has been used directly by Joy UI, Material UI, and MUI X, without going through MUI Base.
One example:
import { unstable_useId as useId } from '@mui/utils';which makes me wonder if it would make sense to:
- treat @mui/utils as an alpha unstable package, private APIs
- reexport everything that is relevant in "@mui/base".
- import from "@mui/base" wherever possible.
- have @mui/utils host all the logic that could be required by @mui/base or @mui/system, only.
in order to better sell the value proposition of MUI Base as a set of baseline helpers to create React components, including React utils.
There was a problem hiding this comment.
I don't remember us deciding on the shape of this library. To me, it should be a private package (I wouldn't say "alpha" or "unstable" - it's just internal).
As for re-exporting from MUI Base - I generally agree, but only the utilities that we intend to make public. I think Material UI and Joy could still import directly from utils.
In the case of this PR, I changed the imports of generateUtilityClass, so that MUI Base could override the generic implementation from @mui/utils and add its own product name. As we ended up not using product classes, after all, this may not be necessary.
We can discuss it in the next meeting.
There was a problem hiding this comment.
To me, it should be a private package (I wouldn't say "alpha" or "unstable" - it's just internal).
@michaldudak I personally prefer this option too.
I think Material UI and Joy could still import directly from utils.
Developers use the source of the components as a reference for how to create new ones. So if we bypass @mui/base in the source, I think that it would mean that @mui/utils is not an internal help for @mui/base and @mui/system but a public API they will install on their own. e.g. what MUI X is doing. But why not, we could imagine that @mui/utils is also part of MUI Base, and documented there too.
Yep, this change is only about Base. |
|
Let's discuss the usage of @mui/utils and whether we should import from Utils or Base or any other package separately. I'm going to merge this PR since there are no other comments related to the class names. |
Make the Base unstyled components' CSS classes follow the same pattern:
BaseComponentName.This change doesn't affect SliderUnstyled and ModalUnstyled as they are used in their respective Material UI components. We don't have the possibility to control the class name prefix per component yet.
Once we implement this feature, we can also rename these two prefixes.
This is a breaking change for anyone who depends on the class names applied to Base components.
If you use the
<component>UnstyledClassesobjects, you won't notice a difference. Only if you depend on the resulting class names (e.g. in CSS stylesheets), you'll have to adjust your code.Closes #33260
https://deploy-preview-33411--material-ui.netlify.app/material-ui/react-button/
https://deploy-preview-33411--material-ui.netlify.app/joy-ui/react-button/
https://deploy-preview-33411--material-ui.netlify.app/base/react-button/#introduction