[Theme] Improve the current approach

[oliviertassinari]

TL;DR

I have seen two possible options:

1. This one
2. Forbid people to nest MuiThemeProvider and to make them call getMuiTheme from scratch each time they want to change it muiTheme. We can still auto document the keys in this case.

The conversation started here #3336.
I have done this PR to illustrate how we could address this issue (and can be merged like this).

What we want

But what is this issue? Well, we want multiple things:

• Be able to customize all the instances of the same component. For instance, we want all the Snackbar to look the same across the all UI
• Be able to document what properties we can changed and how this will look like
• Be able to change the muiTheme on the fly and keeping it immutable
• How know? Maybe someone will want to nest some MuiThemeProvider

What's hard

The hard part here is that we have a baseTheme that is used to compute rawTheme.
Then we need to find a way to merge an old muiTheme with a new one. How do we know which keys of a snackbar need to be recomputed from the baseTheme and which one the user wants to override?
Have a looks at https://github.com/callemall/material-ui/pull/3340/files#diff-3fa66730c1762ffa59a9426f7ab24751R59.

How I'm solving it

The idea is to move the keys for each component out of the muiTheme. The first advantage is to make the getMuiTheme far simpler. People don't have to know what computations will be applied, all they have to know is how component use those keys. The implementation of the getMuiTheme will be as simple as:

export default function getMuiTheme(muiTheme, ...more) {
  muiTheme = merge({
    zIndex,
    isRtl: false,
    userAgent: undefined,
  }, lightBaseTheme, muiTheme, ...more);

  const transformers = [autoprefixer, rtl, callOnce]
    .map((t) => t(muiTheme))
    .filter((t) => t);
  muiTheme.prepareStyles = compose(...transformers);

  return muiTheme;
}

Then, each component needs a function that built an object of keys that can be customized. For instance with the snackbar:

export function getStylesTheme(muiTheme) {
  const palette = muiTheme.palette;

  return Object.assign({
    textColor: palette.alternateTextColor,
    backgroundColor: palette.textColor,
    actionColor: palette.accent1Color,
  }, muiTheme.snackbar);
}

What is awesome with this, is that we can autodocument the customizations point of the Snackbar by calling getStylesTheme with

muiTheme = {
  palette: {
    alternateTextColor: 'palette.alternateTextColor',
    textColor: 'palette.textColor',
    accent1Color: 'palette.accent1Color',
  },
};

This solution is backward compatible but will need some migration.

Regarding performances

We will end up with a bit more of CPU cycles used to compute the style.

[alitaheri]

I think getThemeStyles makes more sense.

[alitaheri]

I really like that idea. 👍 👍

I also have one concern. we should tell people that the values for each component will no longer be available on the theme object. If they relied on this. This can be pretty breaking on so many levels. And we never told people not to touch the theme. I guess this will piss some people off. But it's a must 😁

[oliviertassinari]

we should tell people that the values for each component will no longer be available on the theme object. If they relied on this.

That's a very good point.

[alitaheri]

This will be hard to memoize though -_-

I'll try to come up with something.

[newoga]

The idea is to move the keys for each component out of the muiTheme. The first advantage is to make the getMuiTheme far simpler. People don't have to know what computations will be applied, all they have to know is how component use those keys.

I like this, I think it's much simpler and what I was hoping to achieve with #2509.

Then, each component needs a function that built an object of keys that can be customized. For instance with the snackbar:

I like putting these functions with the component. Out of curiosity, would components have to call this function themselves or would they get the result passed to them somehow?

What is awesome with this, is that we can autodocument the customizations point of the Snackbar by calling getStylesTheme with

Any way we can auto document customizations would be absolutely beautiful and is a must for this project in my opinion 😍

A few questions:

Be able to customize all the instances of the same component. For instance, we want all the Snackbar to look the same across the all UI

1. From a user API perspective, how would this work with this PR?
2. This is a new question that wasn't the original purpose of this discussion, but if we wanted to deprecate theme props, what do you think is the best way to do this?

[alitaheri]

I like putting these functions with the component.

Me too 😍

Out of curiosity, would components have to call this function themselves or would they get the result passed to them somehow?

I'm working on a way these can be passed down as themeStyles that can also be fed into the selectors ( of memoization that I'm working on) to maximize performance 🐎

From a user API perspective, how would this work with this PR?

They just provide a key on the theme object when calling getMuiTheme(...).

see here

Like: getMuiTheme({snackbar: {color: ...}})

This is a new question that wasn't the original purpose of this discussion, but if we wanted to deprecate theme props, what do you think is the best way to do this?

Is that even possible? O.o maybe property getters? but only es5.1 and higher have them -_-

[newoga]

Is that even possible? O.o maybe property getters? but only es5.1 and higher have them -_-

Yeah that's my point, I don't think it is right now which sucks! ES proxies would be nice but I don't know if there's enough support for it. Unless we just pass the theme through some kind of validating function first in development to check for deprecations. Not sure.

[alitaheri]

Unless we just pass the theme through some kind of validating function first in development to check for deprecations. Not sure.

Hmmm interesting... I like that idea 👍 Like how callOnce works 👍

[alitaheri]

@oliviertassinari Let's get this into the alpha before we document the current approach. We shouldn't do any documentation on the previous way if this is to be accepted ( in my book this is awesome 😍 ) Take a look at my comment. If everyone agrees we should merge this and migrate the components under an umbrella issue.

[newoga]

Hey guys. Honestly, for some reason, I'm still not completely settled and happy with any of the theming approaches we've come up with so far (including my own 😆).

That being said, I think this PR achieves at least the following:

1. Fixes the core issue described in [Theme] Simplify getMuiTheme #3336
2. Improves code clarity by moving theme mapping to component files (and as a result significantly simplifies what is stored in muiTheme)
3. Simplifies theme customization for end users using getMuiTheme() function by removing the distinction between baseTheme and muiTheme

I think this PR represents a step in the right direction and I wouldn't argue if we decided to merge and move forward with this.

Note: Don't get me wrong, I like a lot of things about this. I just haven't shaken off that "I feel like we're missing something." feeling yet.

[oliviertassinari]

I'm still not completely settled and happy with any of the theming approaches we've come up with so far (including my own 😆).

Same for me. I'm kind of hesitating on merging this PR 😁.
@newoga I'm happy with 1. and 3. I'm not convinced by 2. I think that we should wait to have a good memoization technique before pushing this forward. We are going to introduce a small performance regression otherwise.

What do you think about removing 2. and updating the documentation before merging this PR?

[alitaheri]

Ok guys. I think it's not bad to hold off this PR a day or 2 longer. I'll have free time for 2 days. I'll keep working on #3268 and get back to this 👍

[alitaheri]

Guys. What do think if we follow @newoga's approach and move this function into the muiThemeable? that way we'll only have one HOC for these concerns.
What I mean is what if we have a themeSelector function passed down to muiThemeable, and muiThemeable would keep the object until the theme on context changes. with each change it will call the function again, keep it as state and never call that function again until context changes again. it will then pass it down as themeStyles and only the needed keys will go there. in the function we can override, select, calculate and pretty much anything we want and it's going to be called only once!

[oliviertassinari]

@alitaheri Yeah, I love this idea, that's so simple 😍.

It looks like @newoga dreams will become a reality 👍.
So, should I move forward by removing 2. (getStylesTheme) and updating the documentation?

[alitaheri]

Sounds good 👍 @newoga's approach was the best in my opinion. it only lacked a single Object.assign 😆 😆

[oliviertassinari]

it only lacked a single Object.assign

That's so true. I'm glad that we had started looking for other solutions. We just failed 😁.
@newoga 👏.

[newoga]

What do you think about removing 2. and updating the documentation before merging this PR?

Yeah let's finish this PR and get that one issue resolved! 👍

Sounds good 👍 @newoga's approach was the best in my opinion. it only lacked a single Object.assign 😆 😆

I'm glad that we had started looking for other solutions. We just failed 😁.

As long as it's something that we all considered, discussed, implemented, and corrected, I consider it our approach, not mine. 😄 Thanks guys!

It sounds like we're getting close, however let's continue to move and evaluate carefully. 👍

[oliviertassinari]

I have done some aggressive changes to the documentation. That's ready to be reviewed 🎉.

[alitaheri]

I'm loving this 😍 😍 I'd say let's go ahead and merge. This is the best approach I can think of.

[newoga]

I have done some aggressive changes to the documentation. That's ready to be reviewed 🎉.

I'm good with the changes! 👍

[oliviertassinari]

Yeah 🎉

[alitaheri]

Awesome 🎉 😍