[RFC] Restructure the keys inside the theme by component name

[mnajdova]

Summary

This RFC proposes slightly different structure to the theme, by moving the components as first key, and nesting the existing keys (overrides, props and variants - will be added with #21648) inside. As we are doing this breaking change, we may even consider renaming the existing keys based on this #21012 (comment) to cssOverrides and defaultProps.

Changes

Current:

const theme = createMuiTheme({
  overrides: {
    MuiButton: {
      root: {
        fontSize: 20,
      },
    },
  },
  props: {
    MuiButton: {
      disableFocusRipple: true,
    },
  },
  variants: {
    MuiButton: [
      {
        props: { variant: 'dashed' },
        styles: { border: '2px dashed grey' },
      }
    ],
  },
});

Proposed:

const theme = createMuiTheme({
  components: {
    MuiButton: {
      cssOverrides: {
        root: {
          fontSize: 20,
        },
      },
      defaultProps: {
        disableFocusRipple: true,
      },
      variants: [
        {
          props: { variant: 'dashed' },
          styles: { border: '2px dashed grey' },
        },
      ],
    },
  },
});

Motivation

Think component first.

With the proposed definition all theme definitions related to one component can be easily discovered and defined together. It's also interesting if later on, developers have a concern about tree-shaking and want to move the configuration from the theme singleton to wrapper components they import.

Drawbacks

Other than the change being breaking, I don't see any other drawback with the proposed restructuring.

Prio-arts

Same approach in

• https://www.vue-tailwind.com/
• https://next.chakra-ui.com/docs/theming/overview#where-to-put-the-style-config

[oliviertassinari]

One element I wonder about, the cssOverrides and styles keys have a similar purpose and structure. They only differ by having "cssOverrides" as a dictionary of "styles". And yet, they have a brand different name. Is there any way we could have them in a closer lexical field?

[mnajdova]

One element I wonder about, the cssOverrides and styles keys have a similar purpose and structure. They only differ by having "cssOverrides" as a dictionary of "styles". And yet, they have a brand different name. Is there any way we could have them in a closer lexical field?

The difference that I see is that, the first ones are overrides for the component's styles, and second ones are styles for the variant specified. So maybe styleOverrides & styles is better working appropriately?

[kgregory]

Does styles allow for overriding component classes for a variant?

[mnajdova]

Does styles allow for overriding component classes for a variant?

Styles is used for defining additional styles for variants and combination of props that did not exists previosly. You can take a look on #21749 for more details

[kgregory]

Styles is used for defining additional styles for variants and combination of props that did not exists previosly. You can take a look on #21749 for more details

I guess my question was more to find out if there was a difference in behavior and it seems there is. styles will add additional rules, overrides allows for overriding the classes in the documented CSS API. If I missed something I’m sorry, but if not, I think the difference in attribute name may be important.

[mnajdova]

I guess my question was more to find out if there was a difference in behavior and it seems there is. styles will add additional rules, overrides allows for overriding the classes in the documented CSS API. If I missed something I’m sorry, but if not, I think the difference in attribute name may be important.

That's why we are proposing styleOverrides for overriding the classes documented, and styles for defining new styles, exactly the -Overrides prefix should help with reflecting this difference. Does this make sense @kgregory ?

[ryancogswell]

I think the overall structure change is nice, but I think it is important to add a function to ease migration such as createMuiThemeFromV4Structure that would take care of reorganizing the props and overrides portion of the passed in theme as necessary. Then developers could import that instead of createMuiTheme if they want to put off reorganizing their theme to the new structure.

[kgregory]

That's why we are proposing styleOverrides for overriding the classes documented, and styles for defining new styles, exactly the -Overrides prefix should help with reflecting this difference. Does this make sense @kgregory ?

Makes sense. I am suggesting that your original proposal is good because cssOverrides deals in class names and styles does not.

[ryancogswell]

If we were to unify the terminology, I think I prefer cssOverrides and css rather than styleOverrides and styles. I like how cssOverrides ties more clearly to the CSS portion of the API documentation. styles makes me think of inline styles which might not give quite the right mental model -- I assume the CSS for a variant will support media queries and pseudo classes (just like you can do within one of the rules of the cssOverrides). I think the styles name might provide an impression that it wouldn't support those aspects (since they aren't supported by inline styles).

[vyushin]

Thanks that you're making MaterialUI better, trying new approaches and improvements.

My thoughts about proposal (after reading comments):

1. createMuiTheme should use v5 theme object;
2. Make method createMuiV4Theme that will use v4 theme object. Show warning in development mode that v4 styles is deprecated and will remove in next versions;
3. Add possibility to write CSS for component classes (see in example below);
4. Rename defaultProps to props. Because when theme is making we understand that each property of theme object will use as default.

const theme = createMuiTheme({
  components: {
    MuiButton: {
      css: { // <-- In this property I would write CSS
        root: {
          fontSize: 20,
          '&:hover, &:focus': { // <-- :hover, :focus, :active, :before, :after etc...
            borderColor: 'green',
          },
          '& span': { // <-- What if I want use span as block inside the button? Depends design requirement
             display: 'block',
           },
          '@media (min-width: 1024px)': { // <-- I can want use media queries
            fontSize: 18
          },
        },
        fullWidth: {
          boxSizing: 'border-box',
          width: '100%',
        },
      },
      props: { // <-- We know that it's default props when make the theme (because all of theme object are default values). So we can use props instead defaultProps
        disableFocusRipple: true,
      },
    },
  },
});

[eps1lon]

I fear that we'll not be able to codemod a lot of existing patterns. We should be extremely careful with handwaving breaking changes as "might as well do it while we're at it". This was a criticism in the most recent survey and change would double down on criticism (again after introducing a new API for customization). I don't understand why we do these surveys if we not only ignore them but directly go against the points made in the answers.

[oliviertassinari]

A couple more comments on the RFC on the replies of https://twitter.com/olivtassinari/status/1286950696298872832.

---

as "might as well do it while we're at it"

@eps1lon How did you conclude that this "thinking approach" is one as the motivation for the change?

This was a criticism in the most recent survey

How did you identify this feedback from our users as frequent (or if we extrapolate, important) from the survey answers?

[eps1lon]

A couple more comments on the RFC on the replies of https://twitter.com/olivtassinari/status/1286950696298872832.

as "might as well do it while we're at it"

@eps1lon How did you conclude that this "thinking approach" is one as the motivation for the change?

Reading

As we are doing this breaking change, we may even consider renaming the existing keys based on this #21012 (comment) to cssOverrides and defaultProps.

-- #21923 (comment)

This was a criticism in the most recent survey

How did you identify this feedback from our users as frequent (or if we extrapolate, important) from the survey answers?

Reading

fewer breaking changes

Under "5. How can we improve Material-UI for you?"

-- https://medium.com/material-ui/2020-material-ui-developer-survey-results-7565085580a8

[oliviertassinari]

fewer breaking changes

@eps1lon A couple of thoughts:

1. The data. We had 7 feedback for fewer breaking changes in 2020. For comparison, we had 1488 participants. We also had 189 feedback for easier customization, which is what we aim for with this change. We will probably have to make more BCs in order to ease customizability.
   During the survey of 2019, in the middle of the v4 BCs effort, we had x5 (relatively) more feedback for fewer breaking changes, which still doesn't make a lot. I think that the data tells us that we shouldn't weight BC concern a lot.

2. The history. From [RFC] Material-UI v5 🚀 #20012 We have had a much more aggressive BCs approach in the past. What's worse than a rewrite while ignoring the past API? But still, things went well overall (even Angular, they still recovered). In any case, we don't plan a rewrite.

the v0 to v1 upgrade was a major breaking change (a rewrite with a brand new API), and yet it was successful.

3. The vision/intuition. From [RFC] Material-UI v5 🚀 #20012. We should be long term oriented (3+ years). Either we evolve with the needs of the community or we slowly become irrelevant. I think that breaking changes have short-term consequences, as long as we provide developers a strong incentive to upgrade and we have a predictable strategy.

I think that we should be willing (taking the risk) to make bold changes, as long as they fit in the direction we see the community going in the long term (with Material-UI empowering it).

[mnajdova]

@eps1lon the main purpose of the RFC is restructuring the keys inside the theme, that's what the name indicates as well. The renaming is added only as consideration.

As we are doing this breaking change, we may even consider renaming the existing keys based on this #21012 (comment) to cssOverrides and defaultProps.

I did not get it whether you disagree with the proposal for moving the keys, or only the renaming part.

If we wanted to rename this in the past, and now we are going to introduce some changes in the object structure anyway, probably now it is the best time to batch this breaking changes together, to avoid in the future two times solving breaking changes in one place. That is the only reason why I proposed this change in the RFC too.

[eps1lon]

The data

At what percentage do we ignore the feedback? Ignoring it after reading the survey makes the data useless.

I think that we should be willing (taking the risk) to make bold changes, as long as they fit in the direction we see the community going in the long term

This statements says nothing. It reads like an advertisement.

[oliviertassinari]

At what percentage do we ignore the feedback?

@eps1lon I don't know. I think that we should focus on the biggest pain points. If feedback X is in opposition to feedback Y, probably we should weight them per frequency? In our case, if easier customization is x27 more frequently asked for than fewer breaking changes, maybe it's a weight we can take into account?

This statement says nothing. It reads like an advertisement.

Agree, to clarify it, I think that we should be long term focused (a couple of years). Breaking changes has a lot of short-term considerations that we should ignore. With two limits. 1. If we make too many of them, people might avoid us for long term considerations of their product development flow (the BCs are not worth it). 2. Any breaking change might motivate a team to consider the alternatives, we take the risk of churn. My cynical view would be, it's a great feedback loop. If people are moving away, it's because we didn't deliver what they truly want. A great way to course-correct, it accelerates learning.

[oliviertassinari]

An interesting approach to customizability that goes in this direction: https://www.vue-tailwind.com/

https://www.vue-tailwind.com/docs/alert

[oliviertassinari]

Another interesting approach to customizability that goes in this direction:

const theme = {
  components: {
    Badge: {
      baseStyle: {
        fontSize: "12px",
        fontWeight: "bold",
        textTransform: "uppercase",
        color: "white",
        px: "2",
        py: "1",
        borderRadius: "4px",
      },
      variants: {
        "in-progress": {
          bg: "blue.600",
        },
        new: {
          bg: "purple.600",
        },
        removed: {
          bg: "red.600",
        },
        default: {
          bg: "gray.600",
        },
      },
      defaultProps: {
        variant: "default",
      },
    },
  },
};

https://next.chakra-ui.com/docs/theming/overview#where-to-put-the-style-config

[mbrookes]

@oliviertassinari I'm missing what value that adds over the proposed API.

[mnajdova]

The only thing missing from these APIs is combination of props, otherwise they look really similar (and I like the components key as first 👍 ) On the second proposal I am not sure if they are changing the actual styles or they have some component level tokens, which may be an interesting thing to explore.