[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 #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:

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

@oliviertassinari @newoga @mbrookes Take a look ;)

[alitaheri]

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

[newoga]

@subjectix I'm excited for this change :+1: ! 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 :+1: :+1:

[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 :+1: :+1:

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"? :grin:

[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?

:laughing: :laughing:

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. :smile:

[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 :scream: :scream: That looks way too hacky. I think we should never have those :grin: 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. :+1: :+1:

[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. :sweat_smile: :sweat_smile:

[alitaheri]

@ntgn81 :smile: I understand. It took me a long time to wrap my head around how React works :grin:

[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 :+1: :+1: :+1:

[ntgn81]

For sure! I learned a lot with this :joy:

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 :smile:

[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 :grin:

[alitaheri]

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

We'll it's not our bug :laughing: Just a patch until it's fixed :smile:

[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.

[oliviertassinari]

We'll it's not our bug :laughing: Just a patch until it's fixed :smile:

Well, I wanted to create a new test to the React repository to make sure it's not our bug. I may found some time to do it.

[alitaheri]

Well, I wanted to create a new test to the react repository to make sure it's not our bug.

Sounds like a great idea. :+1: :+1:

[alitaheri]

@oliviertassinari Do you like this folder structuring? It was painful to do -_- but it's over!

[oliviertassinari]

@subjectix That looks good so far. Thanks for doing it :raised_hands: ! So, It seems that your are using PascalCase instead of camelCase for react component :+1:

Regarding components/pages/components/Tabs/index.jsx can we use components/pages/components/Tabs/Page.jsx instead? Well same for other Page component. I'm really against using index.jsx, that's not straightforward when you are looking for a component source file.

[oliviertassinari]

@subjectix Also, I don't think that we should migrate all of our component in a single PR. I guess that we should open an issue for this work. The quickest we can merge this PR, the better :+1:

[alitaheri]

@oliviertassinari Unfortunately we should. because it must be atomic, no work should be based on a commit while we are doing this. I'll be fast.

regarding omponents/pages/components/Tabs/index.jsx can we call it omponents/pages/components/Tabs/Page.jsx instead

I'd go with index. It's a lot more expressive. index.jsx is where all the logic related to the root of that page is. calling it Page will feel like there is a Page component that no one knows about.

[alitaheri]

@oliviertassinari While we are migrating, no travis build will succeed. We'll have failures all over the place. I'll get this done fast. or at least I'll make it stable fast. give me till morning.

[oliviertassinari]

index.jsx is where all the logic related to the root of that page is. calling it Page will feel like there is a Page component that no one knows about.

I'm absolutly not convinced :grin:.

@oliviertassinari While we are migrating, no travis build will succeed. We'll have failures all over the place. I'll get this done fast. or at least I'll make it stable fast. give me till morning.

I see, in this case, good luck! And try to do as few changes as possible. I'm gonna work on react-native.

[alitaheri]

@oliviertassinari How about the import statement? :P

import Dialog from './components/pages/components/Dialog';

over

import Dialog from './components/pages/components/Dialog/Page';

no? well at least let's do it all together after all pages are migrated. I'll do it myself later. :P

[oliviertassinari]

I would say

import DialogPage from './components/pages/components/Dialog/Page';

and in an ideal world

import DialogPage from './Dialog/Page';

[alitaheri]

:scream: :scream: Alright, I'll change it as the last commit of this PR. remember to remind me xD

DialogPage does make a lot more sense. you win this time... gadget!

[alitaheri]

Now I'm going to migrate the components one by one.

Adding _muiTheme gave me 33 warnings on the first page load. Now we should drop them to zero one by one.

This is where people can help out :grin: make PRs on my fork if you wish to add to this.

[oliviertassinari]

I'm wondering, can we split the migration to the HOC theme context and the update of the documentation? I wish I could edit the doc :grimacing:

[alitaheri]

@oliviertassinari You can base your work on 37433d0 I will not rebase from now on ^_^

[alitaheri]

@newoga @oliviertassinari Try looking at the changes so far. It's 4:11 AM, I can't code anymore. I'll continue tomorrow. and hopefully it's will be finished tomorrow. I take it back, do not base any work on these! I will need to clean it up a lot. and please don't work on the components. only safe place right now is the old doc pages. they aren't changed much :grin: Goodbye World!

[oliviertassinari]

Try looking at the changes so far. It's 4:11 AM, I can't code anymore. I'll continue tomorrow

:clap:

[newoga]

@subjectix Haha! That's good work out of you :+1: You too @oliviertassinari!

I've been away from my computer all weekend, sorry I couldn't have been more help! I'm pretty much caught up with comments but haven't gone over the code. I should have some time tomorrow to take a look more in depth but excited with what I see so far :smile:

[alitaheri]

Thanks guys. ^_^ I'll finish this tonight, after work.

[alitaheri]

@oliviertassinari Please take a look at my last commit. I patched muiThemeable so it can forward imperative methods. We should get rid them asap. but for now this will do.

[oliviertassinari]

@subjectix That's such an epic PR :boom:

[alitaheri]

@oliviertassinari Thanks. still fixing regressions though :grin:

[ntgn81]

@subjectix do those imperative methods ever return anything? If any of them do, don't forget to return them through also

[alitaheri]

@ntgn81 Wow, I forgot! Thanks for pointing that out :+1: :+1: :+1:

[newoga]

Nice catch @ntgn81!

Looks awesome @subjectix!

[alitaheri]

@oliviertassinari I broke every component with this :D :D at least we now know exactly what methods are used throughout the library. I have to include these methods.

[ntgn81]

Glad I could help!

I wonder if there are any performance implications with extra layer of wrapping on every component like this, especially with how many components are wrapped with this change.

Would there be much difference between HOC used here vs directly extending the components themselves (using prototype, or ES6 class extend)?

[alitaheri]

@ntgn81 It will take a much longer time to migrate to es6-classes.

I don't think it would hurt performance. It's like one extra function call. There are many other things that hurt performance a lot more that one layer of abstraction.

[ntgn81]

That's for sure! :+1:

[alitaheri]

Guys I'll need another day to include the imperative methods. Except for a couple of components (Date/Time Picker, Auto-Complete, Theme, tests) everything else works. It's just the methods. All help is appreciated :grin: I need to find all imperative methods and add their name to that list. Please, when ever you get the time go through the changes and tell me if I missed any. I will merge this as soon as all doc pages (at least) work properly.

[ntgn81]

I can definitely get that list for you