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

[Base] Slot class names are inconsistent #33260

Closed
samuelsycamore opened this issue Jun 23, 2022 · 12 comments · Fixed by #33411
Closed

[Base] Slot class names are inconsistent #33260

samuelsycamore opened this issue Jun 23, 2022 · 12 comments · Fixed by #33411
Assignees
Labels
bug 🐛 Something doesn't work package: base-ui Specific to @mui/base

Comments

@samuelsycamore
Copy link
Contributor

samuelsycamore commented Jun 23, 2022

Current behavior 😯

As I've been documenting the component slots and their classes in the MUI Base docs (#33156), I've noticed that the format for class names is inconsistent across the components. Variations on the theme include:

Expected behavior 🤔

I would expect these names to all have the same format.


Personally, I think it should be something like MuiBaseComponentName, e.g. MuiBaseButtonUnstyled, MuiBaseTrapFocus.

@samuelsycamore samuelsycamore added status: waiting for maintainer These issues haven't been looked at yet by a maintainer package: base-ui Specific to @mui/base labels Jun 23, 2022
@michaldudak
Copy link
Member

Thanks for reporting this. AFAIK we settled on BaseComponentName, so we'll have to update the other ones. MuiBaseComponentName is also OK, but the former is a bit shorter and it's pretty clear as well.

@michaldudak michaldudak removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jun 23, 2022
@michaldudak michaldudak self-assigned this Jun 23, 2022
@samuelsycamore
Copy link
Contributor Author

Cool, that naming pattern works for me! 👍

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 6, 2022

AFAIK we settled on BaseComponentName

@michaldudak Is there a discussion about it we can link to?


I wonder if we shouldn't enforce component exported name = theme slot name = class name prefix to make things as predictable as possible. It's almost what has been done historically. (The convention we have trained developers to learn so far). For example, if the import is:

import { BadgeUnstyled } from '@mui/material';

then I would expect the class name to be prefixed with MuiBadgeUnstyled- and the theme slot, well for unstyled, it doesn't make sense. Here is another way to look at it. When a developer sees:

import { InputUnstyled, inputUnstyledClasses } from "@mui/material"

shouldn't he expect inputUnstyledClasses.root to equal 'MuiInputUnstyledClasses-root' rather than 'MuiBaseInput-root'?

It could be a great new conformance test to add to our test suite 👼


Regarding Joy UI and Material UI both use MuiSwitch-root for their class name, is this OK? Maybe, since they should share a very similar DOM/component structure anyway? I mean, the closer the API between the two design systems, the better. I'm not sure that it works at 100% in practice though, it might be a tradeoff. To be clear Mui would be in this context a reference to our company name, not to Material UI.

@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Jul 6, 2022
@samuelsycamore
Copy link
Contributor Author

samuelsycamore commented Jul 10, 2022

@oliviertassinari My vote would be for either .BaseButtonUnstyled-root or .MuiBaseButtonUnstyled-root for the sake of clarity. If I see a class called MuiTrapFocus-root for example, it's not clear if that's a Material UI or an MUI Base component since the name doesn't include Unstyled.

Since Material UI owns Mui*, I would expect Joy to have its own names to differentiate it. Either MuiJoyButton-root or JoyButton-root. I do see the benefits of keeping the same names as Material UI in the interest of making it simpler for Material UI users to move to Joy UI. But if it can't be applied universally, then it's probably not worth trying to do that.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 10, 2022

If I see a class called MuiTrapFocus-root for example, it's not clear if that's a Material UI or an MUI Base component since the name doesn't include Unstyled.

@samuelsycamore This seems to make the assumption that we should optimize for developers to be able to easily know if a component is from MUI Base or Material UI component. Should it be? Why?

=> IMHO, the className names should optimize for making it easier to customize the components.

Regarding Joy UI and Material UI both use MuiSwitch-root for their class name, is this OK?

Actually, they don't: proof https://mui.com/joy-ui/react-switch/#introduction, my expectation was off. Joy UI uses .JoySwitch-root when Material UI uses .MuiSwitch-root.


@michaldudak I think that I have found a different problem. In https://mui.com/joy-ui/react-badge/#introduction the classes on the root is JoyBadge-root BaseBadge-root css-1e1t5y6, shouldn't we remove BaseBadge-root to improve the DX?

@samuelsycamore
Copy link
Contributor Author

@oliviertassinari if there are no downsides to enforcing a single naming convention across all components + libraries (i.e. MuiSlider-root whether it's Material UI, Base, or Joy), then I would be in favor of that option. It's the least complicated solution for sure.

@michaldudak
Copy link
Member

An option could be to enforce MuiComponentName, e.g. MuiSlider for all the components regardless of where they are coming from: MUI Base, Joy UI, Material UI.

It would make it harder to override styles of components from just one package using plain CSS. I'm not sure if it's a sensible use case (i.e. having both Material UI and Base inputs in the same application).

Material UI is an extension of MUI Base, and Joy UI <-> Material UI are meant to be interchangeable

Yes, but if you have Material components and styled .MuiSwitch-root in your CSS, then switched to Joy, you'll likely need to change your CSS to have a good-looking component again. So changing a component with keeping the custom CSS isn't really an option.

For example in https://mui.com/joy-ui/react-badge/#introduction the classes on the root is JoyBadge-root BaseBadge-root css-1e1t5y6, shouldn't we remove BaseBadge-root to improve the DX?

Yes, I agree - that's why we need a way to customize or disable the generation of class names in Base components.


To summarize: I feel like having prefixes for each product would work best. So MuiButton, JoyButton and BaseButton. I also would not include Base classes in Material and Joy components.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 14, 2022

We had a chat with @michaldudak, we have found the following dimensions that seem important to optimize for in the tradeoff:

  • Simplicity of the code/source. The less code, the better.
  • Minimize the DOM bloat. The cleaner the Chrome dev tools are when inspecting an element, the better.
  • Minimize the need to open the dev tools or docs. The more people can work from their IDE, the better.
  • Maximize consistency. The more you can rely on previous knowledge or have to remember things, edge cases, the better.
  • Uniqueness. So that developers can progressively adopt components in a new codebase.

It would make it harder to override styles of components from just one package using plain CSS. I'm not sure if it's a sensible use case (i.e. having both Material UI and Base inputs in the same application).

Yeah, not sure we should optimize for this too much, I would be curious to see what we gain in exchange on giving up on this.

@michaldudak
Copy link
Member

michaldudak commented Aug 25, 2022

@mui/core, I've created a poll to settle this finally: #34069.


@oliviertassinari's edit: While this is about the CSS class name. We might also want to consider the impact for overrides in theme.components.x. Options from this comment.

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'

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.

Downsides:

  • developers who 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.
  • Predicting the value that should be used for theme.components.x is impossible. It's something else to learn.

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.

Downsides:

  • Joy UI have its own unique prefix but Material UI and MUI Base don't. It can feel strange.

C. DesignComponentName

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'

Downsides:

  • sliderUnstyledClasses.thumb = 'BaseSlider-thumb' might feel surprising.

D. ProductComponentName

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'

Downsides:

  • sliderUnstyledClasses.thumb = 'BaseSlider-thumb' might feel surprising.
  • We reinforce that MUI is the contraction of Material UI.

E. 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'

It can feel predictable

Downsides:

  • Since 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, leading to an overhead. The class names are a bit longer than with the alternatives.

F. CompanyComponentName + Product

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 Mui-Md'
<SliderJoy /> // sliderJoyClasses.thumb = 'MuiSlider-thumb Mui-JoyUi'
<SliderUnstyled /> // sliderUnstyledClasses.thumb = 'MuiSlider-thumb Mui-Base'
  • no breaking changes
  • ability to define global overrides per product by increasing the specificity of the styles
  • MUI != Material UI
  • no need to change Base classes when reexporting (other than replacing .Mui-Base with .Mui-Md)

To determine:

  • do we need the Mui-Product class on each slot or just the root?
  • what should be the actual names of the classes

@michaldudak
Copy link
Member

michaldudak commented Sep 12, 2022

The poll results are in:

image

We will proceed with the last option - have Mui[Component] in each product + product-specific classes on root slots.
This solution won't make any breaking changes in Material UI. Plus, it will remove the Mui = Mateiral UI association

@siriwatknp

This comment was marked as outdated.

@michaldudak

This comment was marked as outdated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants