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

[styles] Generate global class names #15140

Merged
merged 10 commits into from Apr 25, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Apr 1, 2019

Closes #15002.

Breaking change

Remove the dangerouslyUseGlobalCSS options (makes it the default behavior).


This change is almost like we were providing default values to the classes API. There is one difference though. We are also using these classes to style the components. The advantage is that you can easily find the class name applying a specific CSS property. Once you know the class name to target, you can override it. It's similar to the strategy Ant Design uses. I have heard good feedback about this proposed change for overriding styles with styled components and SCSS. The disadvantage is that Material-UI has to automatically opt-out the global class names when theme nesting is used. I believe it's better than providing true default values to the classes API (that we wouldn't use to style the component). You can look at Sancho-UI as a library implementing this pattern. It's not ideal.

This is an important structural change. We want to make sure we are going in the right direction. Changing it can have large impacts on our users (once they use this new API). I hope the beta phase will be enough to validate and properly test it.

I have created a new TextField customization example. For complex DOM tree structures, it's simpler, the usage of the classes API can be a mess:

const CssTextField = withStyles({
  root: {
    '& label.focused': {
      color: 'green',
    },
    '& .MuiOutlinedInput-root': {
      '& fieldset': {
        borderColor: 'red',
      },
      '&:hover fieldset': {
        borderColor: 'yellow',
      },
      '&.focused fieldset': {
        borderColor: 'green',
      },
    },
  },
})(TextField);

or with styled components:

Capture d’écran 2019-04-20 à 01 13 30

The RFC

It's a continuation of #14999. I would like to get people feedback on the following change.

  • Start Date: 2019-04-02

Summary

We have many people that find the overrides of our components hard. This is an attempt to improve the situation. We had made this topic one of our priorities in the roadmap. I'm proposing a new global CSS API. I hope it can serve the 20% of the people using plain CSS and the 20% using styled-components.

I have implemented a POC, you can browse the documentation live: https://deploy-preview-15140--material-ui.netlify.com/.

Basic example

  • This was the state of affaire before any effort in improving the overrides:
const StyledTextField = styled(({ className, ...props }) => (
  <TextField
    {...props}
    InputProps={{
      classes: {
        root: className,
        notchedOutline: "outline",
        focused: "focused"
      }
    }}
  />
))`
  & .outline {
    border-color: red;
  }
  &:hover:not(.focused):not(.e):not(.e) .outline {
    border-color: yellow;
  }
  &.focused .outline {
    border-color: green;
  }
`;

https://codesandbox.io/s/zxmxjlx34.

  • This is the new state of affaire with the latest next version:
const StyledTextField = styled(({ className, ...props }) => (
  <TextField
    {...props}
    InputProps={{
      classes: {
        root: className,
        notchedOutline: "outline",
        focused: "focused"
      }
    }}
  />
))`
  & .outline {
    border-color: red;
  }
  &:hover .outline {
    border-color: yellow;
  }
  &.focused .outline {
    border-color: green;
  }
`;

https://codesandbox.io/s/yjrxrj06n9

  • This is the proposed solution:
const StyledTextField = styled(TextField)`
  .mui-outlined-input {
    &.mui-outlined-input--notched-outline {
      border-color: red;
    }
    &:hover .mui-outlined-input--notched-outline {
      border-color: yellow;
    }
    &.focused .mui-outlined-input--notched-outline {
      border-color: green;
    }
  }
`;

https://codesandbox.io/s/xl7w73nq2w

Motivation

Overrides

I hope that by using global CSS, we will make the overrides of the library more accessible.
When first designing the style API of the library, it's something we wanted to avoid doing. We have put the approach under a dangerouslyUseGlobalCSS flag. It's documented as a quick escape hatch for prototyping. I'm proposing to make it the default behavior.

Right now, if you want to customize the components, your best strategy is to:

  1. Open the browser HTML dev tool
  2. Look at the generated class names for the development environement
  3. Reverse the syntax to find the right classes API
  4. Provide the required class names to the classes API
  5. Get the specificity & DOM structure right
  6. Write your CSS

This proposal removes the need for step n°3 and n°4, and helps with n°5 with a shorthand notation for the pseudo class like .disabled, .selected, .checked that requires a specificity of 2 instead of 1.

It should solve #15002 at the same time.

Inspiration

It's simpler to take inspiration of another website when you can revert engineer it's structure. Global class names help.

Detailed design

You can think of this change as providing meaningful default values to the classes API.
I like the strategy used by https://github.com/moroshko/react-autosuggest#theme-optional.
I think that it works better than what react-select tried: #14999 (comment).

The generated class names are close to the BEM methodology. We can take the button as an example:

.mui-button {
  color: rgba(0, 0, 0, 0.87);
  padding: 6px 16px;
  font-size: 0.875rem;
  min-width: 64px;
  box-sizing: border-box;
  transition: background-color 250ms cubic-bezier(0.4, 0, 0.2, 1) 0ms,box-shadow 250ms cubic-bezier(0.4, 0, 0.2, 1) 0ms,border 250ms cubic-bezier(0.4, 0, 0.2, 1) 0ms;
  line-height: 1.75;
  font-family: "Roboto", "Helvetica", "Arial", sans-serif;
  font-weight: 500;
  border-radius: 4px;
  letter-spacing: 0.02857em;
  text-transform: uppercase;
}
.mui-button:hover {
  text-decoration: none;
  background-color: rgba(0, 0, 0, 0.08);
}
.mui-button.disabled {
  color: rgba(0, 0, 0, 0.26);
}
@media (hover: none) {
  .mui-button:hover {
    background-color: transparent;
  }
}
.mui-button:hover.disabled {
  background-color: transparent;
}
.mui-button--label {
  width: 100%;
  display: inherit;
  align-items: inherit;
  justify-content: inherit;
}
.mui-button--text {
  padding: 6px 8px;
}
.mui-button--text-primary {
  color: #2196f3;
}
.mui-button--text-primary:hover {
  background-color: rgba(33, 150, 243, 0.08);
}
@media (hover: none) {
  .mui-button--text-primary:hover {
    background-color: transparent;
  }
}
.mui-button--text-secondary {
  color: rgb(225, 0, 80);
}
.mui-button--text-secondary:hover {
  background-color: rgba(225, 0, 80, 0.08);
}
@media (hover: none) {
  .mui-button--text-secondary:hover {
    background-color: transparent;
  }
}
.mui-button--outlined {
  border: 1px solid rgba(0, 0, 0, 0.23);
  padding: 5px 16px;
}
.mui-button--outlined.disabled {
  border: 1px solid rgba(0, 0, 0, 0.26);
}
.mui-button--outlined-primary {
  color: #2196f3;
  border: 1px solid rgba(33, 150, 243, 0.5);
}
.mui-button--outlined-primary:hover {
  border: 1px solid #2196f3;
  background-color: rgba(33, 150, 243, 0.08);
}
@media (hover: none) {
  .mui-button--outlined-primary:hover {
    background-color: transparent;
  }
}
.mui-button--outlined-secondary {
  color: rgb(225, 0, 80);
  border: 1px solid rgba(225, 0, 80, 0.5);
}
.mui-button--outlined-secondary:hover {
  border: 1px solid rgb(225, 0, 80);
  background-color: rgba(225, 0, 80, 0.08);
}
.mui-button--outlined-secondary.disabled {
  border: 1px solid rgba(0, 0, 0, 0.26);
}
@media (hover: none) {
  .mui-button--outlined-secondary:hover {
    background-color: transparent;
  }
}
.mui-button--contained {
  color: rgba(0, 0, 0, 0.87);
  box-shadow: 0px 1px 5px 0px rgba(0,0,0,0.2),0px 2px 2px 0px rgba(0,0,0,0.14),0px 3px 1px -2px rgba(0,0,0,0.12);
  background-color: #e0e0e0;
}
.mui-button--contained.focusVisible {
  box-shadow: 0px 3px 5px -1px rgba(0,0,0,0.2),0px 6px 10px 0px rgba(0,0,0,0.14),0px 1px 18px 0px rgba(0,0,0,0.12);
}
.mui-button--contained:active {
  box-shadow: 0px 5px 5px -3px rgba(0,0,0,0.2),0px 8px 10px 1px rgba(0,0,0,0.14),0px 3px 14px 2px rgba(0,0,0,0.12);
}
.mui-button--contained.disabled {
  color: rgba(0, 0, 0, 0.26);
  box-shadow: none;
  background-color: rgba(0, 0, 0, 0.12);
}
.mui-button--contained:hover {
  background-color: #d5d5d5;
}
@media (hover: none) {
  .mui-button--contained:hover {
    background-color: #e0e0e0;
  }
}
.mui-button--contained:hover.disabled {
  background-color: rgba(0, 0, 0, 0.12);
}
.mui-button--contained-primary {
  color: #fff;
  background-color: #2196f3;
}
.mui-button--contained-primary:hover {
  background-color: #1976d2;
}
@media (hover: none) {
  .mui-button--contained-primary:hover {
    background-color: #2196f3;
  }
}
.mui-button--contained-secondary {
  color: #fff;
  background-color: rgb(225, 0, 80);
}
.mui-button--contained-secondary:hover {
  background-color: rgb(157, 0, 56);
}
@media (hover: none) {
  .mui-button--contained-secondary:hover {
    background-color: rgb(225, 0, 80);
  }
}
.mui-button--color-inherit {
  color: inherit;
  border-color: currentColor;
}
.mui-button--size-small {
  padding: 4px 8px;
  min-width: 64px;
  font-size: 0.8125rem;
}
.mui-button--size-large {
  padding: 8px 24px;
  font-size: 0.9375rem;
}
.mui-button--full-width {
  width: 100%;
}

Drawbacks

  • The main drawback I can think of is around the major version upgrade path. The advantage of the classes API is that we can warn when the provided string does no longer match a DOM element. This can happen between two major version of Material-UI.
  • It doesn't not support theme nesting. I don't think that we need to support theme nesting.

Alternatives

Instead of making it the default, we could make people opt-in. It's what we do with dangerouslyUseGlobalCSS. In practice, we would make it safe instead of unsafe.

Adoption strategy

This is only a breaking change for people using dangerouslyUseGlobalCSS. We can update the documentation usage one step at the time. I think that we can keep demonstrating the classes API usage without the default values where it's simple enough.

How we teach this

With the documentation. We can document the default class names values in the /api/.

Unresolved questions

?

@mui-pr-bot
Copy link

mui-pr-bot commented Apr 1, 2019

@material-ui/core: parsed: +0.10% , gzip: +0.16%
@material-ui/lab: parsed: +0.19% , gzip: +0.24%
@material-ui/styles: parsed: +0.62% , gzip: +0.89%

Details of bundle changes.

Comparing: 1155be4...35e8f3f

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core +0.10% 🔺 +0.16% 🔺 311,191 311,507 84,408 84,543
@material-ui/core/Paper +0.47% 🔺 +0.69% 🔺 67,308 67,624 19,985 20,123
@material-ui/core/Paper.esm +0.45% 🔺 +0.67% 🔺 60,714 60,988 18,898 19,025
@material-ui/core/Popper 0.00% +0.01% 🔺 31,113 31,114 10,801 10,802
@material-ui/core/Textarea 0.00% +0.08% 🔺 5,472 5,472 2,366 2,368
@material-ui/core/TrapFocus 0.00% +0.06% 🔺 3,731 3,731 1,564 1,565
@material-ui/core/styles/createMuiTheme 0.00% -0.02% 15,943 15,943 5,778 5,777
@material-ui/core/useMediaQuery 0.00% +0.10% 🔺 2,106 2,106 974 975
@material-ui/lab +0.19% 🔺 +0.24% 🔺 140,760 141,034 42,560 42,663
@material-ui/styles +0.62% 🔺 +0.89% 🔺 50,835 51,151 15,023 15,156
@material-ui/system 0.00% 0.00% 11,765 11,765 3,923 3,923
Button +0.32% 🔺 +0.41% 🔺 85,663 85,937 25,640 25,746
Modal 0.00% -0.02% 20,578 20,579 6,609 6,608
colorManipulator 0.00% 0.00% 3,904 3,904 1,543 1,543
docs.landing 0.00% 0.00% 51,154 51,154 11,250 11,250
docs.main +0.07% 🔺 +0.07% 🔺 648,872 649,336 202,332 202,481
packages/material-ui/build/umd/material-ui.production.min.js +0.09% 🔺 +0.13% 🔺 292,685 292,943 82,275 82,385

Generated by 🚫 dangerJS against 35e8f3f

@oliviertassinari

This comment has been minimized.

@eps1lon
Copy link
Member

eps1lon commented Apr 2, 2019

A big part of the discussion is why a CSS-in-JSS was chosen previously. We can't just discuss these issues in isolation while ignoring history. Otherwise we'll be going in circles.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 2, 2019

You can find one part of the history in https://github.com/oliviertassinari/a-journey-toward-better-style: the #Interesting dimensions. I don't see how it's a regression with the considered dimensions. If I had to redo it today, I would consider the performance and overrides potentiel dimensions as well.

@oliviertassinari oliviertassinari self-assigned this Apr 11, 2019
@oliviertassinari oliviertassinari changed the title [RFC] Use global class names [styles] Use global class names Apr 14, 2019
@oliviertassinari oliviertassinari added the package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5. label Apr 14, 2019
@oliviertassinari oliviertassinari force-pushed the rfc-global-class-names branch 6 times, most recently from cb6dac3 to fa07c10 Compare April 15, 2019 22:17
@oliviertassinari oliviertassinari force-pushed the rfc-global-class-names branch 2 times, most recently from 9a1fdbe to 92f280e Compare April 15, 2019 22:25
@oliviertassinari oliviertassinari removed their assignment Apr 15, 2019
@oliviertassinari oliviertassinari force-pushed the rfc-global-class-names branch 3 times, most recently from 25cc354 to 9cc4b99 Compare April 15, 2019 23:12
@oliviertassinari oliviertassinari marked this pull request as ready for review April 15, 2019 23:12
@oliviertassinari oliviertassinari changed the title [styles] Use global class names [styles] Generate global class names Apr 15, 2019
@oliviertassinari oliviertassinari force-pushed the rfc-global-class-names branch 2 times, most recently from 2574943 to 4c237d9 Compare April 15, 2019 23:17
@oliviertassinari oliviertassinari removed their assignment Apr 24, 2019
@ryancogswell
Copy link
Contributor

ryancogswell commented Apr 24, 2019

@oliviertassinari The inexactness of the *= approach makes me uneasy, but as long as the CSS class names take this into consideration such that we never have a case where one class name begins with another class name unless the shorter name is always present on the same element as the longer name (e.g. MuiButton-outlined and MuiButton-outlinedPrimary) then it should be OK.

So with this approach, is it true that the only way to explicitly control the seed for nested themes is to call createGenerateClassName yourself with a seed and then leverage StylesProvider with the resulting generateClassName method? I think that's fine given that the "prefix selector" approach can be used to handle nested themes, but I just want to make sure I'm correctly understanding how to control this. I haven't previously looked through the details of the jss integration and in this first time through I got a little lost trying to follow how to impact options.generateId in makeStyles.js:attach.

@oliviertassinari
Copy link
Member Author

(e.g. MuiButton-outlined and MuiButton-outlinedPrimary) then it should be OK.

A leading dash is added before the index counter. It can be used to discriminate the style rules, well spotted.

then leverage StylesProvider

Yes, it's correct.

@ryancogswell
Copy link
Contributor

@oliviertassinari I do have one concern related to my previous comment. I think the following change causes the prefix selector approach to be unreliable:

const prefix = `${name}${rule.key === 'root' ? '' : `-${rule.key}`}${classNameSeed}`;

Now if I want to target [class*="MuiOutlinedInput"], it is also going to match MuiOutlinedInput-input which resides on a different element. Without the above change, I would be able to safely use [class*="MuiOutlinedInput-root"] without accidentally targeting nested elements.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 24, 2019

I think that you are correct one more time. If the class names were MuiButton-outlined-jss135, MuiButton-outlinedPrimary-jss134 and MuiButton-jss54. I think that we would be fine. You would target .MuiButton without nesting and [class^="MuiButton-jss"] with nesting.

Disabling the root shorhand would work too. I like that better actually.
You would target .MuiButton without nesting and [class^="MuiButton-root-"] with nesting.

@ryancogswell
Copy link
Contributor

You would target .MuiButton without nesting and [class^="MuiButton-root-"] with nesting.

@oliviertassinari Are you meaning you would only disable the root shorthand when in a nested theme? The goal would be to allow customizations to be done in a way that would work regardless of nesting. When supporting nesting, it would be cleaner to be able to use [class^="MuiButton-root"] and have it successfully match "MuiButton-root" when used in a non-nested scenario and successfully match "MuiButton-root-2" (or whatever counter value) when used within a nested theme. If the shorthand "MuiButton" is used when not nested, then there wouldn't be any reliable way to match in both cases except to do something like

.MuiButton, [class^="MuiButton-root-"] {
/* customizations */
}

which would work, but is a lot uglier looking. I remember seeing discussion somewhere recently regarding the shorthand for "root", but I don't remember where or how strongly people felt about this. To me, I think the resulting classes match up more consistently with the component CSS documentation without the shorthand (i.e. leaving "-root" as part of the class name) and it would seem confusing for it to be inconsistent between nested and non-nested scenarios.

oliviertassinari and others added 9 commits April 25, 2019 14:45
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
Co-Authored-By: oliviertassinari <olivier.tassinari@gmail.com>
@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 25, 2019

@ryancogswell I have moved the seed to prefix the generated class name, I have removed the root shortening. We should be good now. The theme nesting story is acceptable. I would encourage people to nest themes as few as possible. When they do, it's better to provide a seed and to target each theme independently: .light-MuiButton-root and .dark-MuiButton-root. If they prefer, they can use a fuzzy class name match: [class*="MuiButton-root"]. It should work everywhere. The case you have mentioned regarding MuiButton-outlinedPrimary and MuiButton-outlined conflict is fine. We only prefix a style rule when it's applied on the same DOM element.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented Apr 25, 2019

This change has a high reward and high risk profile. The future will tell us.

@oliviertassinari oliviertassinari merged commit f1197de into mui:next Apr 25, 2019
@oliviertassinari oliviertassinari deleted the rfc-global-class-names branch April 25, 2019 19:29
@HaNdTriX
Copy link
Contributor

HaNdTriX commented Apr 29, 2019

Just to give you some feedback on this topic.
I just reenabled the setting:

createGenerateClassName({
  disableGlobal: true
})

Reasons:

  • I prefer short classNames (no bundle increase, etc.)
  • I prefer using JSS instead of styled-components
  • I really like the flat way of overwriting styles. I believe this pattern was one of the best parts of Material-UI. It forces you to explicitly overwrite styles without accidentally changing child components.
  • I relied on data-test attributes for e2e tests. It was weird in the beginning but I got used to it.

Since this is a pure developer happiness change, at the expense of performance please keep at least the option to reenable the old behaviour (disableGlobal: true).

@oliviertassinari
Copy link
Member Author

@HaNdTriX Thank you for the feedback! Performance should be pretty close between the two versions. We will definitely keep the disableGlobal option.

@HaNdTriX
Copy link
Contributor

HaNdTriX commented Apr 29, 2019

Just to add some stats:

We had an initial payload increase by 9.9 KB - 10.5 KB => 5.7 % on a simple page.

Source:

@oliviertassinari
Copy link
Member Author

Thanks for sharing these stats!

@Romcol
Copy link

Romcol commented Mar 6, 2020

@ryancogswell I have moved the seed to prefix the generated class name, I have removed the root shortening. We should be good now. The theme nesting story is acceptable. I would encourage people to nest themes as few as possible. When they do, it's better to provide a seed and to target each theme independently: .light-MuiButton-root and .dark-MuiButton-root. If they prefer, they can use a fuzzy class name match: [class*="MuiButton-root"]. It should work everywhere. The case you have mentioned regarding MuiButton-outlinedPrimary and MuiButton-outlined conflict is fine. We only prefix a style rule when it's applied on the same DOM element.

@oliviertassinari I do uncounter the same challenge when using overriding with global class names when using seeds. My styles with elements targeted such as this & .MuiDialogTitle-root .MuiTypography-root have stopped working since the elements are now such as this MyPrefix-MuiDialogTitle-root. I wonder what is the best way to override those nested elements. Sometimes it's hard to use the classes API (doesn't target all wanted elements). Maybe with theme nesting or should it be avoided? Thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change package: styles Specific to @mui/styles. Legacy package, @material-ui/styled-engine is taking over in v5.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Able to reference nested component in Theme
8 participants