[Toolbar] Remove style-propable mixin

[newoga]

@oliviertassinari @alitaheri @mbrookes

This isn't done, as I'm migrating the code, I wanted to get your feedback on this pattern of moving getStyles() outside of the class declaration (and changing the method signature to getStyles(props, state).

I think this approach has the following benefits:

1. Reduces component surface API (and imperative methods) which is safer and allows us to move more towards functional components
2. Prevents leaky abstractions and prevents developers from implementing custom styling and behavior from undocumented methods (Disclosure: I am not trying to say anything bad about this developer 😁. I perfectly understand what and why he was doing what he was doing, but I still think we should find a better way for material-ui to handle his use case and prevent developers from falling into the trap of using internal API)
3. Discourages the use of other instance methods to calculate style such this.getTheme() and this.getSpacing()
4. Forces us (and other contributors) to think of style as a function of props and state (and ideally in the future just a function of props)
5. It's more functional, predictable, and reusable (could open up interesting opportunities in the future like composition and more)

Final note: This PR was originally the start of migrating the toolbar components off of style-propable. After we decide, I'll rename the PR and finish that work.

Signed-off-by: Neil Gabbadon [email protected]

[oliviertassinari]

@newoga I think that it's an excellent idea!

[newoga]

@newoga I think that it's an excellent idea!

I'm glad, I think this change would make me very happy. 😆

[alitaheri]

Pure awesomeness and conciseness 👍 👍 This will bring us very close to memoization 😍

[newoga]

Pure awesomeness and conciseness 👍 👍 This will bring us very close to memoization 😍

Let's do it! 😄 🎉

I applied that pattern to the rest of the components under ./toolbar. Take a look please 😁

[newoga]

I want to draw your attention here for feed back.

The previous implementation was recomputed the whole style object for each property on mouse enter and leave:

_handleMouseEnterFontIcon(e) {
  e.target.style.zIndex = this.getStyles().icon.hover.zIndex;
e.target.style.color = this.getStyles().icon.hover.color;
},

I replaced it with a higher order function so that the style object is only computed once per render. Honestly, if I had my way, we would move these outside of the class too! 😁

If these were removed, the only time this. is called is for these three scenarios:

1. this.props
2. this.setState({muiTheme: aMuiTheme})
3. this.state.muiTheme

It's so close to being purely functional!

[alitaheri]

Yeah! That would be great. But it's going to need a lot of thought. I think an HOC that can handle these interactions would make this stateless (an a whole lot of other components) but I don't know how that is possible.

[newoga]

Done, change setState to nextContext.muiTheme || this.state.muiTheme

[alitaheri]

I see you're doing this almost everywhere you use prepareStyles maybe it's time to move the logic before we merge these? @oliviertassinari What do you think?

[oliviertassinari]

I don't follow. What would be the advantage?

[alitaheri]

well since it's gonna show up everywhere, why not just put it in the prepareStyles function in the first place. this way we won't have to make the assumption that the styles passed down to prepareStyles are safe to mutate. we'll just make them safe ourselves.

[oliviertassinari]

That's fair. I see a performance concern in this approach. 50%+ of the time, the parameter passed to the prepareStyles is safe to mutate. I feel like we will end up with more Object.assign({}, with your solution.

However, in this line I think that prepareStyles(Object.assign(styles.root, style)) would work just fine. But that's not 100% safe for future code changes (side effect).

[alitaheri]

I think more than 50% would be safe, you're right. Let's go with this approach for now 👍

[alitaheri]

I'm all good. feel free to merge 👍