Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Toolbar] Remove style-propable mixin #3180

Merged
merged 1 commit into from
Feb 5, 2016
Merged

[Toolbar] Remove style-propable mixin #3180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
173 changes: 86 additions & 87 deletions src/toolbar/toolbar-group.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,72 @@
import React from 'react';
import Colors from '../styles/colors';
import StylePropable from '../mixins/style-propable';
import getMuiTheme from '../styles/getMuiTheme';

function getStyles(props, state) {
const {
firstChild,
float,
lastChild,
} = props;

const {
baseTheme,
button,
toolbar,
} = state.muiTheme;

const marginHorizontal = baseTheme.spacing.desktopGutter;
const marginVertical = (toolbar.height - button.height) / 2;

const styles = {
root: {
float,
position: 'relative',
marginLeft: firstChild ? -marginHorizontal : undefined,
marginRight: lastChild ? -marginHorizontal : undefined,
},
dropDownMenu: {
root: {
float: 'left',
color: Colors.lightBlack, // removes hover color change, we want to keep it
display: 'inline-block',
marginRight: baseTheme.spacing.desktopGutter,
},
controlBg: {
backgroundColor: toolbar.menuHoverColor,
borderRadius: 0,
},
underline: {
display: 'none',
},
},
button: {
float: 'left',
margin: marginVertical + 'px ' + marginHorizontal + 'px',
position: 'relative',
},
icon: {
root: {
float: 'left',
cursor: 'pointer',
color: toolbar.iconColor,
lineHeight: toolbar.height + 'px',
paddingLeft: baseTheme.spacing.desktopGutter,
},
hover: {
color: Colors.darkBlack,
},
},
span: {
float: 'left',
color: toolbar.iconColor,
lineHeight: toolbar.height + 'px',
},
};

return styles;
}

const ToolbarGroup = React.createClass({
propTypes: {
/**
Expand Down Expand Up @@ -42,13 +106,10 @@ const ToolbarGroup = React.createClass({
muiTheme: React.PropTypes.object,
},

//for passing default theme context to children
childContextTypes: {
muiTheme: React.PropTypes.object,
},

mixins: [StylePropable],

getDefaultProps() {
return {
firstChild: false,
Expand All @@ -69,87 +130,20 @@ const ToolbarGroup = React.createClass({
};
},

//to update theme inside state whenever a new theme is passed down
//from the parent / owner using context
componentWillReceiveProps(nextProps, nextContext) {
const newMuiTheme = nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme;
this.setState({muiTheme: newMuiTheme});
this.setState({
muiTheme: nextContext.muiTheme || this.state.muiTheme,
});
},

getTheme() {
return this.state.muiTheme.toolbar;
_handleMouseEnterFontIcon: (style) => (e) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

e.target.style.zIndex = style.hover.zIndex;
e.target.style.color = style.hover.color;
},

getSpacing() {
return this.state.muiTheme.rawTheme.spacing;
},

getStyles() {
const {
firstChild,
float,
lastChild,
} = this.props;

const marginHorizontal = this.getSpacing().desktopGutter;
const marginVertical = (this.getTheme().height - this.state.muiTheme.button.height) / 2;
const styles = {
root: {
float,
position: 'relative',
marginLeft: firstChild ? -marginHorizontal : undefined,
marginRight: lastChild ? -marginHorizontal : undefined,
},
dropDownMenu: {
root: {
float: 'left',
color: Colors.lightBlack, // removes hover color change, we want to keep it
display: 'inline-block',
marginRight: this.getSpacing().desktopGutter,
},
controlBg: {
backgroundColor: this.getTheme().menuHoverColor,
borderRadius: 0,
},
underline: {
display: 'none',
},
},
button: {
float: 'left',
margin: marginVertical + 'px ' + marginHorizontal + 'px',
position: 'relative',
},
icon: {
root: {
float: 'left',
cursor: 'pointer',
color: this.getTheme().iconColor,
lineHeight: this.getTheme().height + 'px',
paddingLeft: this.getSpacing().desktopGutter,
},
hover: {
color: Colors.darkBlack,
},
},
span: {
float: 'left',
color: this.getTheme().iconColor,
lineHeight: this.getTheme().height + 'px',
},
};

return styles;
},

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

_handleMouseLeaveFontIcon(e) {
_handleMouseLeaveFontIcon: (style) => (e) => {
e.target.style.zIndex = 'auto';
e.target.style.color = this.getStyles().icon.root.color;
e.target.style.color = style.root.color;
},

render() {
Expand All @@ -160,7 +154,12 @@ const ToolbarGroup = React.createClass({
...other,
} = this.props;

const styles = this.getStyles();
const {
prepareStyles,
} = this.state.muiTheme;

const styles = getStyles(this.props, this.state);

const newChildren = React.Children.map(children, currentChild => {
if (!currentChild) {
return null;
Expand All @@ -171,33 +170,33 @@ const ToolbarGroup = React.createClass({
switch (currentChild.type.displayName) {
case 'DropDownMenu' :
return React.cloneElement(currentChild, {
style: this.mergeStyles(styles.dropDownMenu.root, currentChild.props.style),
style: Object.assign({}, styles.dropDownMenu.root, currentChild.props.style),
styleControlBg: styles.dropDownMenu.controlBg,
styleUnderline: styles.dropDownMenu.underline,
});
case 'RaisedButton' :
case 'FlatButton' :
return React.cloneElement(currentChild, {
style: this.mergeStyles(styles.button, currentChild.props.style),
style: Object.assign({}, styles.button, currentChild.props.style),
});
case 'FontIcon' :
return React.cloneElement(currentChild, {
style: this.mergeStyles(styles.icon.root, currentChild.props.style),
onMouseEnter: this._handleMouseEnterFontIcon,
onMouseLeave: this._handleMouseLeaveFontIcon,
style: Object.assign({}, styles.icon.root, currentChild.props.style),
onMouseEnter: this._handleMouseEnterFontIcon(styles.icon),
onMouseLeave: this._handleMouseLeaveFontIcon(styles.icon),
});
case 'ToolbarSeparator' :
case 'ToolbarTitle' :
return React.cloneElement(currentChild, {
style: this.mergeStyles(styles.span, currentChild.props.style),
style: Object.assign({}, styles.span, currentChild.props.style),
});
default:
return currentChild;
}
}, this);

return (
<div {...other} className={className} style={this.prepareStyles(styles.root, style)}>
<div {...other} className={className} style={prepareStyles(Object.assign({}, styles.root, style))}>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{newChildren}
</div>
);
Expand Down
61 changes: 28 additions & 33 deletions src/toolbar/toolbar-separator.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,25 @@
import React from 'react';
import StylePropable from '../mixins/style-propable';
import getMuiTheme from '../styles/getMuiTheme';

function getStyles(props, state) {
const {
baseTheme,
toolbar,
} = state.muiTheme;

return {
root: {
backgroundColor: toolbar.separatorColor,
display: 'inline-block',
height: baseTheme.spacing.desktopGutterMore,
marginLeft: baseTheme.spacing.desktopGutter,
position: 'relative',
top: ((toolbar.height - baseTheme.spacing.desktopGutterMore) / 2),
width: 1,
},
};
}

const ToolbarSeparator = React.createClass({

propTypes: {
Expand All @@ -20,13 +38,10 @@ const ToolbarSeparator = React.createClass({
muiTheme: React.PropTypes.object,
},

//for passing default theme context to children
childContextTypes: {
muiTheme: React.PropTypes.object,
},

mixins: [StylePropable],

getInitialState() {
return {
muiTheme: this.context.muiTheme || getMuiTheme(),
Expand All @@ -39,47 +54,27 @@ const ToolbarSeparator = React.createClass({
};
},

//to update theme inside state whenever a new theme is passed down
//from the parent / owner using context
componentWillReceiveProps(nextProps, nextContext) {
const newMuiTheme = nextContext.muiTheme ? nextContext.muiTheme : this.state.muiTheme;
this.setState({muiTheme: newMuiTheme});
},

getTheme() {
return this.state.muiTheme.toolbar;
},

getSpacing() {
return this.state.muiTheme.rawTheme.spacing;
},

getStyles() {
return {
root: {
backgroundColor: this.getTheme().separatorColor,
display: 'inline-block',
height: this.getSpacing().desktopGutterMore,
marginLeft: this.getSpacing().desktopGutter,
position: 'relative',
top: ((this.getTheme().height - this.getSpacing().desktopGutterMore) / 2),
width: 1,
},
};
this.setState({
muiTheme: nextContext.muiTheme || this.state.muiTheme,
});
},

render() {

const {
className,
style,
...other,
} = this.props;

const styles = this.getStyles();
const {
prepareStyles,
} = this.state.muiTheme;

const styles = getStyles(this.props, this.state);

return (
<span {...other} className={className} style={this.prepareStyles(styles.root, style)}/>
<span {...other} className={className} style={prepareStyles(Object.assign({}, styles.root, style))}/>
);
},

Expand Down
Loading