[WIP] Major Theme Handling Migration

[alitaheri]

This is a continuation of #2541 that handles some other issues as well. But this is incomplete I wish to migrate the entire codebase to use HOC theme wrappers written by @newoga as well (Thanks a lot ^_^). However, before doing that I would like to discuss some matters that are very essential on how this PR will continue.
Closes #2460

1. Do we agree on having the internal name for muiTheme renamed to _muiTheme that is, every component will eventually receive a prop called _muiTheme. Yes! A discussion on this is at Proposal: Use underscore for internal props #2580
2. I included lodash.merge for deep merging baseTheme. For this reason, there will be no need to write ThemeManager.getMuiTheme(DefaultRawTheme), ThemeManager.getMuiTheme() does the same thing. That is because an empty object is merged with the default Theme and then with the input of this function. simplifies a lot of things. Does everyone agree on this behavior? It doesn't break anything, but rather, it prevents future regressions caused by adding props to baseThemes. accepted

TODO:

• Migrate all the components to use the Wrappers.
  • App Bar
  • Auto Complete
  • Avatar
  • Badge
  • Button
  • Card
  • Data Picker
  • Dialog
  • Divider
  • DropDownMenu
  • GridList
  • Icon
  • Icon Button
  • Icon Menu
  • LeftNav
  • Lists
  • Menus
  • Paper
  • Popover
  • Progress
  • Refresh Indicator
  • Select Field
  • Slider
  • Switch
  • Snackbar
  • Table
  • Tabs
  • Text Field
  • Time Picker
  • Toolbars
• Replace Style mixin with Util functions I will leave this for another PR
• Migrate the documentation.
• Add info on this in the documentation. Another PR

@oliviertassinari @newoga @mbrookes Take a look ;)

[alitaheri]

this is not a jsx file, so I changed it to React.createElement. + I think {muiTheme, ...props} is better, allows you to override the theme through the props. might come in handy :D

[newoga]

@subjectix Yes, this is better, thanks! 😄

[alitaheri]

Cool 😁 😁

[alitaheri]

I also migrated 2 components as a demonstration of how awesome muiThemeable is 😁

[newoga]

@subjectix I'm excited for this change 👍 ! This should help reduce a lot of the cognitive overhead in developing and maintaining components and reduce the LOC for each component too.

I'll review this in more detail later and report back. Some initial thoughts:

1. Do we agree on having the internal name for muiTheme renamed to _muiTheme

My gut feeling is to leave it as muiTheme for now, though I could change my mind. I'd like to think a little bit more about the concept of internal fields, research how other projects might handle this problem, and consider what other internal fields we may have in the near or long term. Also, we may want to consider as you suggested in one of your code comments if we want to support allowing users to override muiTheme using the prop, in that case it wouldn't be internal anymore. So for now I'd say let's work on finding a way to hide it from documentation (similar to how the React team just didn't initially document the context feature).

1. I included lodash.merge for deep merging baseTheme.

Should we consider using immutable.js for calculating/merging/comparing themes objects (as opposed to lodash)? I haven't given it much thought myself but I read at #1176 that defining themes as immutable structures could help improve or open up the possibility for some performance optimizations. This PR seems like a natural place to include that change if we want to do that.

[ntgn81]

This is awesome! So much code can be removed. I was going the exact same path after reading @newoga's PR, trying to learn React at the same time.

Here are the issues I found, that you might not be addressing yet, @subjectix :

1. HOC hides the public methods the components might be exposing. Dialog - according to documentation (probably outdated?) - exposes isOpen method. It will be hidden once wrapped.

   • Might not be the best way, but what I did to fix this was to expose statics.publicMethods from the components, telling the HOC what to expose/proxy through. I'm taking DatePickerDialog as sample, as it has show/hide methods. Then in the HOC, I ref the child component, then proxying the public methods through that ref.

     const DatePickerDialog = React.createClass({
       statics: {
         publicMethods: ['show', 'dismiss'],
       },
     }

     This, I think, as a side-effect, can make it clearer to the users what's public and what's not - easier to document.

2. It's hard to expose properties from components (are there such components at the moment?)

My stab at it, mostly similar, with the addition of proxying public methods is at

https://github.com/ntgn81/material-ui/pull/1

[alitaheri]

in one of your code comments if we want to support allowing users to override muiTheme using the prop, in that case it wouldn't be internal anymore

I didn't mean expose this to public :D I meant in some cases, maybe we want override the theme INTERNALLY there is a bug with dialog, it doesn't pass down context, maybe we want to get theme from context and provide it with props. There might be cases like this around. I was just keeping an open mind. exposing this as prop to public is dangerous and very confusing, everyone will be like: y no theme for nested children when I set the theme prop on a higher one? y u do this?

Should we consider using immutable.js for calculating/merging/comparing themes objects

We don't merge and update themes so much. It's no more than possibly once, per page load. and immutable.js is a big library. I use it in every project I work on. but it's not needed here in my opinion. that performance problem can be easily fixed with memoization. I'm against adding immutable.js as a dependency to this project. The themes are created once or twice per load! and the styles will be fixed with simple memoization. But I'm surely open minded to reconsider. You do have pretty good ideas 👍 👍

[alitaheri]

@ntgn81 Thank you for the insights. You have a point. However, we are deprecating imperative methods. So we'll re expose them for now, to provide a little patch. I really like your proxy pattern 👍 👍

It's hard to expose properties from components (are there such components at the moment?)

I don't understand. can you please explain? what do you mean by "exposing properties"? 😁

[newoga]

I didn't mean expose this to public :D I meant in some cases, maybe we want override the theme INTERNALLY there is a bug with dialog,

Got it, I better understand what you mean now.

everyone will be like: y no theme for nested children when I set the theme prop on a higher one? y u do this?

😆 😆

I use it in every project I work on. but it's not needed here in my opinion. that performance problem can be easily fixed with memoization. I'm against adding immutable.js as a dependency to this project.

Sounds good to me. Like I said, I haven't given it too much thought and I don't have enough experience with immutable.js to make a strong case for or against it so I trust your opinion. In my opinion, the less dependencies the better. 😄

[newoga]

@ntgn81 Good catch! I hadn't considered those implications. I agree with @subjectix that long term it's better to deprecate those methods and find better and more React-like/idiomatic approaches to doing the same things.

[ntgn81]

I don't understand. can you please explain? what do you mean by "exposing properties"?

@subjectix , like this - https://github.com/callemall/material-ui/blob/master/src/dialog.jsx#L403, or this https://github.com/callemall/material-ui/blob/master/src/DropDownMenu/DropDownMenu.jsx#L235.

doSomething() {
  console.log(this.refs.comp.style);
}

render() {
  <SomeMuiComponent ref="comp" />
}

Not an issue if we don't have these kind of things :D

[alitaheri]

@ntgn81 😱 😱 That looks way too hacky. I think we should never have those 😁 Thanks for the heads up! I'll keep an eye for them.

P.S. The code in Dialog isn't using the prop it's using the actual style property on the dom element, so that won't be effected. The one in the DropDownMenu is deprecated, and will be removed, until then I'll try to fix it. Thanks a lot for your feedback, it helped me a lot understanding the implication of using HOC. 👍 👍

[ntgn81]

Lol, I hoped you'd tell me there's a magical way to make it work through HOC. Glad I was useful!

I'll try to dig through DatePicker and it's dependencies to make theming work all the way. The way it is right now, even on documentation site, popup calendar stays in light theme even after clicking dark.

It should have became dark too since it's a child of DatePicker, right?

http://www.material-ui.com/#/customization/themes

[alitaheri]

@ntgn81

Lol, I hoped you'd tell me there's a magical way to make it work through HOC.

I'm not sure what the use-case would be. I mean the props are passed down to children. If they have props they have it from your render method. and if it's in the render method you can access in much easier ways. Can you give me an example of how this would be beneficial? I might be able to find a way to make it happen.

It should have became dark too since it's a child of DatePicker, right?

It might be caused by some unstable react methods used to make RenderToLayer possible, maybe passing theme as child through the portal fixes this.

[ntgn81]

'm not sure what the use-case would be. I mean the props are passed down to children. If they have props they have it from your render method. and if it's in the render method you can access in much easier ways. Can you give me an example of how this would be beneficial? I might be able to find a way to make it happen.

Oh sorry, I was just trying to be funny. Especially with how noobish I am with React that there's something I totally missed. 😅 😅

[alitaheri]

@ntgn81 😄 I understand. It took me a long time to wrap my head around how React works 😁

[ntgn81]

Got it! Looks like render-to-layer renders the dialog on it's own top level container, so it stays on the default theme and does not receive the theme change triggered from inside the Tab - I'm using customization/themes page to test

Once I the getMuiTheme method in decorator to look into, in order: props.muiTheme -> context.muiTheme -> default, it all works well and the theme gets passed down nicely.

    getChildContext() {
      return {
        muiTheme: this._getMuiTheme(),
      };
    },

    render() {
      const muiTheme = this._getMuiTheme();
      return React.createElement(WrappedComponent, {...this.props, muiTheme, ref} );
    },

    _getMuiTheme() {
      return this.props.muiTheme || this.context.muiTheme || getDefaultTheme();
    },

[alitaheri]

@ntgn81 Interesting! Thanks for sharing 👍 👍 👍

[ntgn81]

For sure! I learned a lot with this 😂

Applied the new decorator to everything under date-picker on my fork's PR and all seems to be working well!

[alitaheri]

@ntgn81 Glad to hear it works. thanks 😄

[oliviertassinari]

Could we add a second parameter called muiTheme?
That would override some value of the outputted muiTheme.
Right now, when we want to change some value of the outputted muiTheme, we have to use, for instance

muiTheme.appBar = Object.assign({}, muiTheme.appBar, {
  textColor: colors.white,
  height: 56,
});

[alitaheri]

I like your idea 👍 👍 yeah, I'll figure out a clean way to do this. thanks 👍

[oliviertassinari]

1. Do we agree on having the internal name for muiTheme renamed to _muiTheme

I don't really have side on this. Both approaches are fine to me.

1. I included lodash.merge for deep merging baseTheme

I think that it's great. Regarding using Immutable.js, I don't think that it's a good idea. If someone need to update the muiTheme, he can call themeManager.getMuiTheme().
This will return a new object reference, so we are fine with a future improved version of ContextPureMixin.

It might be caused by some unstable react methods used to make RenderToLayer possible, maybe passing theme as child through the portal fixes this.

It does fix it, but it would be better to work with the context 😁

[alitaheri]

It does fix it, but it would be better to work with the context

We'll it's not our bug 😆 Just a patch until it's fixed 😄

[alitaheri]

About the naming, I think at least for muiTheme it's better to use underscore. it's much more expressive. I'll go with underscore. and update the commits, tell me how you will like them.

[ntgn81]

@subjectix That was one last comment on that file, I promise 🙏

Sorry for being nosey, I just switched my project completely to React with this. I want nothing but for this project to keep kicking more butts 🚀

[alitaheri]

@ntgn81 Feedbacks are always welcome, so no worries at all 👍 Thanks a lot for your insights. 👍 👍

[ntgn81]

Can remove this dependency now ^.^

[alitaheri]

no we need this for merging themes. assign won't help us there

[alitaheri]

@ntgn81 Your list list saved me hours! Thanks a lot 🙏 🙏

[alitaheri]

@oliviertassinari I don't know why, but this PR also fixed Dialog not receiving The parent theme O.O I have no idea how. but it works now in the customization page of my last commit 😆

[alitaheri]

@newoga @oliviertassinari @ntgn81 @mbrookes This is final with 2 minor issues:

1. Auto complete seems broken, But I rather not fix it here, it should use Popover anyway.
2. Some deprecation warnings

I could fix these, But I think merging this so other works can continue takes higher priority.

Also I need you guys to go through the changes and look out for regressions. Thanks 👍 🎉

[alitaheri]

I see all green after so long 🎉 🎉

@oliviertassinari I would rather keep the history as it is. rebasing cleanly is impossible, I can make this at most 10 clean commits which would make huge commits. That will severely hurt tracking changes if something comes up. I advice against squshing these commits. These are major changes, they should be easily tracked. But I'm open to discussion 😁 😁

Also, I haven't forgotten about Page.jsx over index.jsx. I'll do it another PR.

[oliviertassinari]

I don't know why, but this PR also fixed Dialog not receiving The parent theme O.O

That's because the _muiTheme property is passed down, we bypass the context 😁.

<DialogInline {...this.props} />

I would rather keep the history as it is

That's going to be 70 commits, I'm not against it. I would say that you can make some commit as fixup during an interactive rebase. You could endup with like 20 commits.

I see all green after so long

🎉

I need you guys to go through the changes and look out for regressions.

I'm gonna run my app tests base on this branch

[newoga]

I'm scared. 😅

I'll preface by saying that l'll go with whatever you and @oliviertassinari agree to do. Even with all of the separate commits, this branch/PR is already really tough to review because of then number of changes 😄 😄

I don't think this should all be squashed down to one commit, though I don't think we should have 70 commits either. I think if I had a choice, I would prefer if some of the core changes were implemented as a separate PRs first, then we could have have a big migration PR that pretty much changes context components to muiThemeable.js. For example:

1. One PR that has improvements to theme-manager.js(with the deep merging implementation)
2. Another PR that has improvements to the muiThemeable.js HOC (with the muiTheme prop name change and the forwarded methods change)
3. Another PR that has the migration from rawTheme to baseTheme
4. Then a final migration of components muiThemeable.js(any components that have regressions or issues like AutoComplete can be left out of this PR and tracked in a separate umbrella issue)

Maybe it's too much work to extract now at this point, but those are my 2 cents! 😄

[oliviertassinari]

@newoga I agree, this would have been better with smaller PR. But since it's already done. Let's see.
@subjectix My tests are green, but I had two issues with this PR.

My custom context is not passed down correctly (I see the default one).

I had to replace

  childContextTypes: {
    muiTheme: React.PropTypes.object,
  },
  getChildContext() {
    return {
      muiTheme: muiTheme,
    };
  },

with

  childContextTypes: {
    _muiTheme: React.PropTypes.object,
  },
  getChildContext() {
    return {
      _muiTheme: muiTheme,
    };
  },

Why did you add an underscore to the context and not just the property?

muiThemeble() is breaking everything when using react-transform-catch-errors.

  return function wrapToCatchErrors(ReactClass, componentId) {
    var originalRender = ReactClass.prototype.render; // ReactClass.prototype is undefined

(https://github.com/gaearon/react-transform-catch-errors/blob/master/src/index.js#L12)

First thing noticed, Object.assign isn't transformed by babel. Don't use it, this should be better:

  return React.createClass({
    displayName: getDisplayName(WrappedComponent),
    contextTypes: {
      _muiTheme: React.PropTypes.object,
    },
    render() {
      const {_muiTheme = getDefaultTheme()} = this.context;
      return React.createElement(WrappedComponent, {
        _muiTheme,
        ref: forwardMethods ? 'WrappedComponent' : undefined,
        ...this.props,
      });
    },
    ...methods,
  });

But, this doesn't keep the prototype either. Here is my dirty fix:

  var myClassBase = {
    displayName: getDisplayName(WrappedComponent),
    contextTypes: {
      _muiTheme: _react2['default'].PropTypes.object
    },
    render: function render() {
      var _context$_muiTheme = this.context._muiTheme;

      var _muiTheme = _context$_muiTheme === undefined ? getDefaultTheme() : _context$_muiTheme;

      return _react2['default'].createElement(WrappedComponent, _extends({
        _muiTheme: _muiTheme,
        ref: forwardMethods ? 'WrappedComponent' : undefined
      }, this.props));
    }
  };
  var myClass = _extends(myClassBase, methods);

  myClassBase.prototype = myClassBase;

  return _react2['default'].createClass(myClass);

So, as long as we address my two points and that we squash down some commit.
I'm 👍 to merge this PR.

[alitaheri]

@newoga I feel ya 😨 😨 I'll try to figure out the best way to do this. I think your idea is good.

Why did you add an underscore to the context and not just the property?

So they all looked the same, I can change it, won't be so hard. Want me to change it?

I agree, this would have been better with smaller PR. But since it's already done. Let's see.

I can do that. Should I close this and make separate PRs?

First thing noticed, Object.assign isn't transformed by babel.

how about the old merge back? might that fix it?

[alitaheri]

I'm going to break this down into smaller PRs:

1. Improve the docs naming convention
2. Improve theme-manager
3. Improvements to the Theme wrapper components.
4. Migrate the components

sounds good?