Skip to content

Include pure-render-decorator #174

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

Merged
merged 5 commits into from
Dec 18, 2015

Conversation

marcusvmsa
Copy link
Contributor

Including pure render decorator as proposed in #141 and organizing imports.

@@ -73,7 +76,7 @@ class CommentForm extends React.Component {
if (this.state.formMode < 2) {
ref = this.refs.text.getInputDOMNode();
} else {
ref = React.findDOMNode(this.refs.inlineText);
ref = ReactDOM.findDOMNode(this.refs.inlineText);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fixes a warning from React 0.14

@justin808
Copy link
Member

@MaMute This looks quite good!

@alexfedoseev Please take a peek at this! Will this break with Babel 6? @MaMute would you mind trying Babel 6. Supposedly, they just fixed some things that are critical to make this work. @alexfedoseev can tell you more.

@alex35mil
Copy link
Member

Yesterday we discussed it and decided to stick with this solution for now, until decorators spec will be on stage 0 at least. @MaMute will update this PR.

@marcusvmsa marcusvmsa force-pushed the include-pure-render-decorator branch from e7da899 to 21d088f Compare December 15, 2015 15:50
@marcusvmsa
Copy link
Contributor Author

@justin808 @alexfedoseev this is ready

@alex35mil
Copy link
Member

@MaMute The idea is to create a parent Component with shouldComponentUpdate method:

/* Component.jsx */

class Component extends React.Component {
  shouldComponentUpdate() {
    return React.addons.PureRenderMixin.shouldComponentUpdate.apply(this, arguments);
  }
}

And then:

import Component from './Component';

export default class EachComponentInApp extends Component {

  // using `shouldComponentUpdate` from Component here...

}

@marcusvmsa marcusvmsa force-pushed the include-pure-render-decorator branch from 21d088f to 4d95cf4 Compare December 15, 2015 21:14
@marcusvmsa
Copy link
Contributor Author

@alexfedoseev Done 👍

@alex35mil
Copy link
Member

@MaMute Looks good 👍 One thing: let's place it in client/app/libs/components so it will be shared among all bundles.

@marcusvmsa
Copy link
Contributor Author

@alexfedoseev done 👍

@justin808
Copy link
Member

@MaMute This is awesome. One last question. Is there any way we can have a test or something that confirms we're getting some benefit from this change?

justin808 added a commit that referenced this pull request Dec 18, 2015
@justin808 justin808 merged commit ca98a44 into shakacode:master Dec 18, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants