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.

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

merged 1 commit into from
Sep 24, 2015

Conversation

oliviertassinari
Copy link
Member

Related to #1176.

@oliviertassinari
Copy link
Member Author

@shaurya947 Done.

@@ -28,6 +32,26 @@ const IconButton = React.createClass({
};
},

statics: {
getContextProps(context) {
Copy link
Member Author

Choose a reason for hiding this comment

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

ousp

shaurya947 added a commit that referenced this pull request Sep 24, 2015
@shaurya947 shaurya947 merged commit 20ddf37 into mui:master Sep 24, 2015
@shaurya947
Copy link
Contributor

Thanks @oliviertassinari!

@oliviertassinari oliviertassinari deleted the iconbutton-pure branch September 24, 2015 15:12
@oliviertassinari oliviertassinari restored the iconbutton-pure branch September 24, 2015 15:12
@@ -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.

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.

@oliviertassinari oliviertassinari deleted the iconbutton-pure branch November 3, 2015 10:51
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Nov 10, 2020
* Rename MuiPickersUtilsPorvider => LocalizationProvider

* Improve utils typings

* Implement global format override

* [docs]  Change name of date-io customization page

* Update examples to include new wording

* Fix prop-types typescript error

* Add daetAdapter prop for passing date-io utils directly to component

* Update percy and cypress

* Unskip flaky test

* Fix typo in error message

* Fix cypress test

* One more try to fix flaky test

* Remove flaky test

* Fix visual regression scenarios tests with new version of cypress

* Run cypress tests in chrome

* Ignore dark theme change in snapshots

* Try to fix example styles one more time

* Fix inccorect name of cypress executor

* Try weird hack to reinject styles

* Optimize theme toggling for visual regression

* Update scenario names to get rid of duplications

* Rename `adapter` => `dateAdapter`

* Rename libFormats => dateFormats, libInstance => dateLibInstance

* Remove version-specific code from README.md

* Fix documentation erros in installation guide

* Run prettier on README.md

* Use edge="end" for keyboard adornment icons, closes mui#1545
@zannager zannager added the component: icon button This is the name of the generic UI component, not the React module! label Mar 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: icon button This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants