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

[IconButton] add purerender mixin #1545

Merged
merged 1 commit into from
Sep 24, 2015
Merged

[IconButton] add purerender mixin #1545

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
49 changes: 38 additions & 11 deletions src/icon-button.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
const React = require('react');
const StylePropable = require('./mixins/style-propable');
const ContextPure = require('./mixins/context-pure');
const Transitions = require('./styles/transitions');
const PropTypes = require('./utils/prop-types');
const EnhancedButton = require('./enhanced-button');
Expand All @@ -11,7 +12,10 @@ const ThemeManager = require('./styles/theme-manager');

const IconButton = React.createClass({

mixins: [StylePropable],
mixins: [
StylePropable,
ContextPure,
],

contextTypes: {
muiTheme: React.PropTypes.object,
Expand All @@ -28,6 +32,26 @@ const IconButton = React.createClass({
};
},

statics: {
getRelevantContextKeys(muiTheme) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@shaurya947 I was using the context props before. I completely overlook this during the rebase! Is muiTheme working?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say that this doesn't work. I think that we need to pass context.muiTheme instead of context to the getRelevantContextKeys in the pure-render.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, exactly what I was thinking. The argument needs to be changed in the context-pure mixin. Could you submit a PR for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

and what about https://github.com/callemall/material-ui/blob/master/src/buttons/flat-button-label.jsx#L44 that doesn't use any argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

That also needs to be changed to look like this:

getRelevantContextKeys(muiTheme) {
    return {
      spacingDesktopGutterLess: muiTheme.rawTheme.spacing.desktopGutterLess,
    };
  },

and call it using this.state.muiTheme wherever it's called in the file. Please let me know if you find any more incorrect definitions of these functions in the source. Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I'm gonna make a PR for this

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, I've merged the new theming documentation in #1716. So make sure to rebase if you need to!

Copy link
Contributor

Choose a reason for hiding this comment

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

It's also in src/card/card-expandable.jsx and src/text-field.jsx

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeap. Question. Is the context required to render a component?

Copy link
Contributor

Choose a reason for hiding this comment

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

I've added you to a room in Gitter: shaurya947/context-pure-fix

Let's chat about this there.

const spacing = muiTheme.rawTheme.spacing;
const palette = muiTheme.rawTheme.palette;

return {
iconSize: spacing.iconSize,
textColor: palette.textColor,
disabledColor: palette.disabledColor,
}
},
getChildrenClasses() {
return [
EnhancedButton,
FontIcon,
Tooltip,
];
},
},

propTypes: {
className: React.PropTypes.string,
disabled: React.PropTypes.bool,
Expand Down Expand Up @@ -64,36 +88,39 @@ const IconButton = React.createClass({
},

getStyles() {
let spacing = this.state.muiTheme.rawTheme.spacing;
let palette = this.state.muiTheme.rawTheme.palette;
const {
iconSize,
textColor,
disabledColor,
} = this.constructor.getRelevantContextKeys(this.state.muiTheme);

let styles = {
root: {
position: 'relative',
boxSizing: 'border-box',
transition: Transitions.easeOut(),
padding: spacing.iconSize / 2,
width: spacing.iconSize * 2,
height: spacing.iconSize * 2,
padding: iconSize / 2,
width: iconSize * 2,
height: iconSize * 2,
fontSize: 0,
},
tooltip: {
boxSizing: 'border-box',
},
icon: {
color: palette.textColor,
fill: palette.textColor,
color: textColor,
fill: textColor,
},
overlay: {
position: 'relative',
top: 0,
width: '100%',
height: '100%',
background: palette.disabledColor,
background: disabledColor,
},
disabled: {
color: palette.disabledColor,
fill: palette.disabledColor,
color: disabledColor,
fill: disabledColor,
},
};

Expand Down