Theme manager improvements

[alitaheri]

rawTheme was renamed to baseTheme. also as of now, the default theme is merged with baseTheme so adding new properties won't break existing code. another notable feature is the possibility to override the calculated theme with the second argument.

Fraction of #2585
Closes #2529

[oliviertassinari]

I'm wondering, shouldn't we use something like https://github.com/rackt/react-redux/blob/master/src/components/connect.js#L76?

[newoga]

The muiThemeable function serves the same purpose as the wrapWithConnect function from react-redux (I actually used that project as an inspiration when creating this one). The main difference between this implementation and connect is that connect accepts parameters, and returns a function that wraps the component, so:

connect(...args)(OurComponent);

/* As opposed to... */
muiThemeable(OurComponent);

In #2509 I created a version of muiThemeable that behaves more like connect that accepts arguments for mapping a baseTheme to localized style/theme objects (objects that only have the style information that the component needs).

[oliviertassinari]

Do you think we should use extends Component { instead of a function?
Here, we are using a createClass https://github.com/callemall/material-ui/blob/master/src/hoc/selectable-enhance.js#L8

In #2509 I created a version of muiThemeable that behaves more like connect that accepts arguments for mapping a baseTheme to localized style/theme objects (objects that only have the style information that the component needs).

Could you explain the benefit of this approach? I can't see it 😁.

[newoga]

In our case it probably doesn't matter too much if its a function or class, it'd be a matter of style. I think for react-redux it was necessary because they needed access to component lifecycle methods. If we needed that as well, I'd say we should use a class but using function vs class shouldn't have any practical implications so I'm open to either (though personally I tend to write things as functions until I need them to be classes).

Could you explain the benefit of this approach? I can't see it 😁.

No problem! 😄 My main intentions behind that other PR were the following:

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

I feel like I'm not presenting my arguments very clearly 😝 if you'd like me to elaborate, I can do so on the #2509 PR.

[newoga]

@oliviertassinari Actually, one more thought on function vs class. @neverfox brought up that there are still some kinks being worked out as it relates to hot-reloading components that are defined as functions. So another reason to switch to a class would be if we're experiencing issues with that. I think you mentioned that the catch errors broke with components decorated with muiTheme? I'd be curious if a switch to class would fix that.

That all being said, I'm pretty sure the last PR/branch you were testing wasn't implemented with a function but was implemented with React.createClass. Could be wrong though...

[newoga]

That was linked to Object.assign that remove the prototype. I wouldn't worry about this here.

Okay cool.

Now that I think about it, even if we don't implement muiTheme as desribed in #2509. It could make sense to make this muiThemeable work like this:

muiTheme()(WrappedComponent);

That way in the future if we ever decide to add arguments that change the behavior of the HOC, we wouldn't have to go through and fix all the components again.

[oliviertassinari]

I feel like I'm not presenting my arguments very clearly 😝 if you'd like me to elaborate, I can do so on the #2509 PR.

It clear enough. Adding it to #2509 PR would be great.

@newoga Are you ok to merge this PR and to continue the discussion?

[newoga]

@oliviertassinari Absolutely! 👍

I think there's a lot we can benefit from in this iteration's implementation and newer more invasive approaches (such as the one discussed in #2509) should be discussed and considered carefully.

[alitaheri]

There is also one more issue regarding function components (That's a better name), They cannot be assigned a ref. That's why in my original PR I switched from function to createClass. But I feel like this isn't right. I have to do more research before I make it final. I'm glad that PR wasn't merged 😁 We shouldn't rush things 😄

[newoga]

@subjectix Yes, that point on "ref" is true.

[alitaheri]

@newoga @oliviertassinari May I merge this?

[newoga]

@subjectix @oliviertassinari I'm thinking we should merge the theme-manager deep merging functionality and renaming on the themes and put a hold on muiThemeable until we finish fleshing out discussing all our options in #2641 and #2509.

Also I agree with @subjectix with not rushing things. Since muiThemeable and related ideas don't have too much impact on the users, maybe we should consider discussing it in depth, consider blockers or other factors we have to consider before implementing it, then implementing it as part of a later release.

[alitaheri]

@newoga My other PR is on the es6-classes branch. This has no conflict with that whatsoever. That branch is there to tackle and test es6-classes before we decide we wish to merge them with master.

Also the rename is not breaking. not even deprecated. this is safe to merge, and will fix critical regressions we have to address before 0.14.0 😁

[newoga]

@subjectix Yes, I understand and agree this is safe! I'm just wondering since we haven't fleshed out whether we want to use muiThemeable.js or figured out what else needs to be done with it if we decide to continue down that path, if it makes sense to do it as part of a separate PR.

[alitaheri]

@newoga we'll remove it if we don't want it 👍 as always: no doc === no commitment. It's safe. don't worry. software is prune to change, we shouldn't worry about having some useless files around (that is if we decide on inheritance)

[alitaheri]

@oliviertassinari Merge this if your ok with it, thanks 😁

[oliviertassinari]

@subjectix Could you let the muiThemeable.jsx untouched in this PR until we decide who we solve it?

[alitaheri]

@oliviertassinari Done.

[oliviertassinari]

@subjectix Thanks 👍

[oliviertassinari]

This was before the PR on my project:

import colors from 'material-ui/lib/styles/colors';
import themeManager from 'material-ui/lib/styles/theme-manager';

import defaultRawTheme from 'material-ui/lib/styles/raw-themes/lightBaseTheme';
defaultRawTheme.palette = Object.assign({}, defaultRawTheme.palette, {
  primary1Color: colors.green500,
  primary2Color: colors.green700,
  primary3Color: colors.green100,
  accent1Color: colors.red500,
});

const muiTheme = themeManager.getMuiTheme(defaultRawTheme);
muiTheme.appBar = Object.assign({}, muiTheme.appBar, {
  height: 56,
});
muiTheme.avatar = Object.assign({}, muiTheme.avatar, {
  borderColor: null,
});

export default muiTheme;

Now, this is how I can write it for the same result:

import colors from 'material-ui/lib/styles/colors';
import themeManager from 'material-ui/lib/styles/theme-manager';

const muiTheme = themeManager.getMuiTheme({
  palette: {
    primary1Color: colors.green500,
    primary2Color: colors.green700,
    primary3Color: colors.green100,
    accent1Color: colors.red500,
  },
}, {
  appBar: {
    height: 56,
  },
  avatar: {
    borderColor: null,
  },
});

export default muiTheme;

Great work!

[newoga]

Beautiful! 🎉

[alitaheri]

Thanks everyone for your feedbacks on this 👍 👍 🎉 🎈

[alitaheri]

@oliviertassinari you could take it one step further too 😁

import themeManager from 'material-ui/lib/styles/theme-manager';
//...
const muiTheme = themeManager.getMuiTheme({

=>

import getMuiTheme from 'material-ui/lib/styles/getMuiTheme';
//...
const muiTheme = getMuiTheme({