[WIP] Proposal: Parameterized mui-themeable hoc

[newoga]

This is an extension of #2493. This pull request explores approaches to not needing a ThemeManager at all (at least in user space).

Note: Once again, just throwing this out there for discussion.

This idea is inspired react-redux. In this pull request, the mui-themeable component created in #2493 now accepts a mapping function that computes a calculated theme from a rawTheme object (simliar to react-redux's connect higher order component's mapStateToProps function).

For example, currently text-field.jsx accepts a muiTheme object in context, then uses calculated theme provided by ThemeManager at the key of textField. It would get the computed result of:

      textField: {
        textColor: rawTheme.palette.textColor,
        hintColor: rawTheme.palette.disabledColor,
        floatingLabelColor: rawTheme.palette.textColor,
        disabledTextColor: rawTheme.palette.disabledColor,
        errorColor: Colors.red500,
        focusColor: rawTheme.palette.primary1Color,
        backgroundColor: 'transparent',
        borderColor: rawTheme.palette.borderColor,
      },

In this pull request's implementation, in text-field.jsx, you could decorate TextField like this:

TextField = muiThemeable((rawTheme) => ({
  textColor: rawTheme.palette.textColor,
  hintColor: rawTheme.palette.disabledColor,
  floatingLabelColor: rawTheme.palette.textColor,
  disabledTextColor: rawTheme.palette.disabledColor,
  errorColor: Colors.red500,
  focusColor: rawTheme.palette.primary1Color,
  backgroundColor: 'transparent',
  borderColor: rawTheme.palette.borderColor,
}))(TextField);

This means that when users supply a theme, they would only need to supply a rawTheme:

import DefaultRawTheme from './styles/raw-themes/light-raw-theme';

const App = (props) => {
  return (
    <ThemeProvider muiTheme={DefaultRawTheme}>
      {props.children} 
    </ThemeProvider>
  );
};

[alitaheri]

@newoga This is a very good idea, it can also drastically improve performance through memoization, but there is only one thing we will have to take care of, how would the users on this library isolate their customization? I mean what if someone wants to change only TextField's hintColor and not touch anything else. right now it's possible. but with this, I'm not so sure. Or is there a way to do that too?

[oliviertassinari]

what if someone wants to change only TextField's hintColor and not touch anything else

That's a very good question that we need to address.

[newoga]

@subjectix @oliviertassinari That is true. This pull request's implementation does not currently support overriding the theme for a particular component. If you two don't mind, I'd like to work with this a little bit more to see if there's a clean way to handle that use case using this approach.

[newoga]

Made these arguments as to why this PR should be considered as a comment in #2635. I can/will flesh out and clarify anything that is unclear.

1. Removes the need for ThemeManager. It's one less thing the user needs to know how to use. I think it'd be more ideal if they knew they could just define their baseTheme as a raw JSON object and pass it directly to a ThemeProvider component, and then our library would know how to properly merge and/or setup the computed muiTheme.
2. I think it makes more sense that the code that maps the baseTheme configuration for a component's section in the muiTheme belongs with the corresponding Component.jsx file, not separately in ./styles/getMuiTheme.js
3. I think it might make more sense that a component that is decorated with muiTheme only receives style properties that are relevant for it, not the whole muiTheme object (that contains style properties for all components). And in addition, these properties could be documented and possible validated against used PropTypes

[alitaheri]

Removes the need for ThemeManager. It's one less thing the user needs to know how to use. I think it'd be more ideal if they knew they could just define their baseTheme as a raw JSON object and pass it directly to a ThemeProvider component, and then our library would know how to properly merge and/or setup the computed muiTheme.

I absolutely agree. I think MuiThemeProvider should take 2 props that it would then pass as arguments to getMuiTheme.

I think it makes more sense that the code that maps the baseTheme configuration for a component's section in the muiTheme belongs with the corresponding Component.jsx file, not separately in ./styles/getMuiTheme.js

It does make more sense, but I don't know about the impact. How would the user customize the entire app as they wish in a single place?

I think it might make more sense that a component that is decorated with muiTheme only receives style properties that are relevant for it, not the whole muiTheme object (that contains style properties for all components). And in addition, these properties could be documented and possible validated against used PropTypes

This would pollute the props the component takes in. Also, It will be harder to maintain since any new style from the theme to want to add to the component should be added in 2 places, and removed from 2 places when you want to remove it, I'm against this one. But having the logic nearby is good. I just can't figure out a convenient way to do this.

[newoga]

It does make more sense, but I don't know about the impact. How would the user customize the entire app as they wish in a single place?

This is from a maintainer perspective not from a user perspective. The user would continue customizing the entire app change the baseTheme. Though right now, the getMuiTheme object has keys for each component. For example, the textField key in the object created in getMuiTheme.js (previously theme-manager.js) has:

      textField: {
        textColor: rawTheme.palette.textColor,
        hintColor: rawTheme.palette.disabledColor,
        floatingLabelColor: rawTheme.palette.textColor,
        disabledTextColor: rawTheme.palette.disabledColor,
        errorColor: Colors.red500,
        focusColor: rawTheme.palette.primary1Color,
        backgroundColor: 'transparent',
        borderColor: rawTheme.palette.borderColor,
      },

I think that code belongs in TextField.jsx, so a developer doesn't have to look in two places to see what is defined here. It's all properly placed in the component file.

This would pollute the props the component takes in.

I'm not sure I follow, how is this different than it currently is? For example, using my example above, my suggestion is that rather than the muiTheme prop containing the whole object with all the component keys, it really just contains an object at the textField key.

Also, It will be harder to maintain since any new style from the theme to want to add to the component should be added in 2 places, and removed from 2 places when you want to remove it, I'm against this one. But having the logic nearby is good. I just can't figure out a convenient way to do this.

I'm not sure I follow or maybe I'm thinking the opposite 😄. Using the example above, I'm thinking if TextField got a new style/theme property, we'd only have to modify TextField.jsx, not TextField.jsx and getMuiTheme.js.

[oliviertassinari]

Thanks for the extra bit of information but I still don't understand how we will address this question #2509 (comment).
Actually I see a way.
For instance, we can let people adding a textField property to the rawTheme. We would merge the custom values with the default ones extracted from the rawTheme.
With this in place, that sounds like a good idea to follow this path 👍.
So now. How would we implement an efficient shouldComponentUpdate?

[alitaheri]

@newoga Sorry I misunderstood some of your points, I thought you meant... nevermind, what I thought was too sick 😅 😅 I take the full fault on this :D

There is only one thing. how can we override TextField related styles Throughout the library and not just for 1 single instance of it?

[newoga]

@subjectix @oliviertassinari First of all, I apologize! 😄 When I initially made this proposal, it was an extension of another PR and I never appropriately or clearly explained any of it. Sorry for all the confusion from the start.

Like I said, my original proposal does not account for overriding a specific component's style across the entire library. I do have a few ideas of how this could work, but I haven't articulated them yet. One of those ideas is what @oliviertassinari alluded to:

For instance, we can let people adding a textField property to the rawTheme. We would merge the custom values with the default ones extracted from the rawTheme.

Another approach is to allow developers to import a non-decorated version of TextField into their own projects. This way they could decorate the component themselves inject their own style (through a similar mapping function) based on the component's style documentation (using or ignoring the passed rawTheme). That way they could have their own version of TextField that they export from within their own project and use throughout their own application.

[alitaheri]

I do agree with having these calculations close to their corresponding components ! I DO agree that the mapper should be close! But NOT excluded from muiTheme!

Just to add another reason to why inheritance is better. you can pass a theme mapper to the constructor of MuiComponent like this:

class MuiComponent extends React.Component {
  constructor(p,c, options) {
    if(options.themeMapper) {
       this.themeMapper = options.themeMapper;
    }
  }
  get theme() {
    if(this.themeMapper) {
      return this.themeMapper(this.context.muiTheme || MuiComponent.DEFAULT_THEME);
    } else {
      return this.context.muiTheme || MuiComponent.DEFAULT_THEME;
    }
  }
}

then you can use it like this:

// do all the heavy calculations in the same file!!
// but let the getMuiTheme RUN it
const themeCalculator = (theme) => {
  // The implementation is debatable :D 
  theme.textField: {
    color: ColorManipulator.doSomeHeavyColorCalculations(theme.baseTheme.userColor)!
    // do all the other calculations necessary.
};

// Export this calculator so getMuiTheme can use this instead of it's own, 
// This will let you provide calculations in the same component! 
// The PICKUP can also be automated, maybe webpack magic? :D :D
export themeCalculator;

// This is a selector! and embrace yourselves!!! 
// MuiComponent can memoize it <3 <3 <3
// That means, one step closer to fully optimized styling
const themeMapper = (theme) => ({
  return {...theme.textField, tooltipZIndex: theme.baseTheme.zIndex.tooltip };
})

class MyComponent extends MuiComponent {
  constructor(p,c) {
    super(p, c, {themeMapper}); // Pass the mapper for memoization and selection.
}

  render() {
    // By only looking at the first two functions in this file you
    // know exactly where these props come from.
    this.theme.tooltipZIndex; // Profit!
    this.theme.color // More Profit!!
  }

[alitaheri]

Another approach is to allow developers to import a non-decorated version of TextField into their own projects. This way they could decorate the component themselves inject their own style based on the component's style documentation (using or ignoring the passed rawTheme). That way they could have their own version of TextField that export from within their own project and use throughout their own application library.

Then we will have to expose the implementation details as public API. It can really hurt maintenance 😬

[newoga]

I wish we could all get together, grab some good food, and have a nice debate! 😄

I clearly see the benefits of the inheritance approach that you proposed, but even so, I fear that using the approach is taking a step away from the general movement of the react community and not towards it. The React (and javascript community as a whole) is a constantly evolving one. I think it's also important to consider how certain design decisions affect our ability to evolve as quickly and easily as everything else easier. My initial impression from the community, including authors of React, is that composition is encouraged over inheritance and is the sole reason why I have a hesitation.

That all being said, I'm fine exploring the inheritance approach in the es6-classes branch like you suggested.

In my opinion, our goal should be to make material-ui components that render consistently given a set of props, and nothing more. I think those components should be documented and tested well, and all of them should publicly accessible regardless of whether they decide to use theming or not. The ability to theme at the application level (or all components in a given subtree) should be considered a separate feature that is handled by high order components (or another approach), but not logically baked into the components themselves (as they would be if everything extends MuiComponent).

For example, if someone is user space decided they wanted to implement their own way on how theming works, it should literally be a matter of them wrapping material-ui components themselves and making them available in their own application. I think this is very much React-ish.

In addition, if someone decided they wanted to use only TextField from this library, I think they should be free to use it without knowing or having to use themes at all. As long as they pass it the appropriate props, it will work (and look) as expected.

Then we will have to expose the implementation details as public API. It can really hurt maintenance 😬

I wouldn't consider them implementation details. To me, the component's theme/styling should potentially be considered as part of it's public API. The component styles are passed through props and would be documented like any other other ordinary prop.

[alitaheri]

I wish we could all get together, grab some good food, and have a nice debate! 😄

You said it 😁 😁 😁

composition is encouraged over inheritance and is the sole reason why I have a hesitation.

Yes!! I completely agree with composition over inheritance. But that is for extending functionality, providing abstraction and much much more!! But NOT for cross-cutting-concerns ( ref ) these cannot all be handled with composition, especially when the coupling is tight. Besides this is only an implementation detail we will discourage the users of this library with a NO-NO. and for implementation detail inheritance is the optimal decision imo.

Sometime you have to resort to Platform Specific Machine Code to get the job done. I don't like doing this all over, I did it once with composition and it just sits there waiting to be merged. But I gave it way too much thought! and the best I came up with was inheritance.

[newoga]

@subjectix Your arguments are fair. Let's experiment, discuss, and find out together!

[alitaheri]

@newoga Thanks 😄 😄 Yeah I would agree with you. I can't be sure myself with no experience on how this would turn out 👍 👍

[mbrookes]

Not the same as sharing a meal together, but why don't you guys and Olivier get together on a google hangout or similar.

@subjectix, been watching your mammoth themes overhaul from the sidelines- Amazing work!

[alitaheri]

@mbrookes That wouldn't be a bad idea 😁

been watching your mammoth themes overhaul from the sidelines- Amazing work!

Thanks a lot ^.^

[newoga]

@subjectix @oliviertassinari @mbrookes I'm open to that!

I'm in EST (but I've had my fair share of 3am nights, and I know @subjectix has had his fair share of his 😄 ). If you want to orchestrate this, let me know when is best.

[newoga]

@ntgn81 Just cc'ing you here in case you're interested, want to make sure everyone is included.

[alitaheri]

@newoga I'm in GTM +3:30 Iran, Tehran. That makes 8:30 hours difference 😁 I'm usually up at nights working on this. around this time :D so I'm alright with it. Just point me to a service and I'll make an account 😎

[newoga]

@subjectix Okay cool, I'm on Google and Skype! Let's see if @oliviertassinari can carve out some time for this!

[alitaheri]

@newoga Yeah he seems really busy recently 😁 I hope he can attend, We must have many discussions about many decisions. not just themes.

It's 3:20 AM here, I should wake up at 8:30. Farewell for now 💤

[oliviertassinari]

I have definitely time to spend so that we can find out a good solution 👍.
I'm not busy, I'm on holiday right now.

[newoga]

@oliviertassinari Sounds good. Github needs a timezone calculator tool 😄 You're in Paris, right? You and @subjectix are only 2.5 hours apart. Maybe when you both wake up, you two can coordinate some times and let me know when is best. I should be up pretty late so I may be up too when you both wake up.

[alitaheri]

I'm not busy, I'm on holiday right now.

I see 😁 Have a nice time 🍊 🎋

Github needs a timezone calculator tool

Aye to that! 😆

I'll leave it to @oliviertassinari and you @newoga Since I can almost always be able to chat. Both at work and at home.

[mbrookes]

Seems like 10am Eastern, 4pm CET, 6:30pm GMT+3:30 would be sociable hours for everyone, depending where @ntgn81 is based. Just not tomorrow! ;)

Might be easiest to schedule via email. Addresses are in the commit history. :)

[ntgn81]

I am EST. Don't let me hold you guys up though. I'll try my best to be available whatever time is good for everyone. It's super hectic with all the year end stuff for me right now lol

[alitaheri]

@newoga Think we can close this? I'm working on my react-memo to support style objects with both static styles and dynamic-memoized.

[newoga]

@newoga Think we can close this? I'm working on my react-memo to support style objects with both static styles and dynamic-memoized.

Yes, thanks!