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

Updating Thinking in React doc to replace refs with event handlers #8815

Merged
merged 2 commits into from Jan 17, 2017
Merged

Updating Thinking in React doc to replace refs with event handlers #8815

merged 2 commits into from Jan 17, 2017

Conversation

rohannair
Copy link

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

One problem with this is sending props “up” can potentially swallow some updates. This is not the case in this particular example but I’d like to avoid this pattern if possible. In React, this.props are the “currently rendered” props but they may not be up to date if there was a scheduled update that has not yet been flushed.

Can you also try creating another version with two separate callbacks so that we could compare them?

@rohannair
Copy link
Author

Will do shortly.

@rohannair
Copy link
Author

@gaearon the second version is here: http://codepen.io/rohan10/pen/xgRLqJ

IMHO, while it's a lot more verbose, I think it does get the concept across in a more digestible manner.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

Yea, I like it better. Let's use it?

@rohannair
Copy link
Author

@gaearon the CodePen has been updated, and I also adjusted the copy slightly to reflect that there are multiple callbacks passed in.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

    this.props.onInStockInput(
      e.target.value
    );

You probably want e.target.checked here. I can't uncheck it.

Also maybe it's not worth using newlines here:

    this.props.onInStockInput(e.target.checked);

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Need to fix the checkbox.

@rohannair
Copy link
Author

Fixed.

@gaearon
Copy link
Collaborator

gaearon commented Jan 17, 2017

Thank you!

@rohannair rohannair deleted the update-thinking-in-react-docs branch January 17, 2017 18:22
aweary pushed a commit that referenced this pull request Jan 18, 2017
…8815)

* Updating Thinking in React doc to replace refs

* Updating doc copy to reflect changes to example

(cherry picked from commit 5d96162)
@gaearon
Copy link
Collaborator

gaearon commented Feb 13, 2017

Oops: #8996

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants