Skip to content

Conversation

@chenglou
Copy link
Contributor

@chenglou chenglou commented Dec 2, 2013

Continuation of #602.

@fabiomcosta
Copy link
Contributor

I would suggest adding it to the current page (http://facebook.github.io/react/tips/communicate-between-components.html) as "case 2" or something similar.

@chenglou
Copy link
Contributor Author

Thanks for reminding me this is still here. Like I said in #602, joining the two really bloats up the tip entry too much, and I haven't found a way to title this appropriately for discoverability. I'm getting on it soon.

@fabiomcosta
Copy link
Contributor

Alright! Thx for improving the documentation!

@vjeux
Copy link
Contributor

vjeux commented Dec 28, 2013

Is it good to be taken in?

@chenglou
Copy link
Contributor Author

Gimme a little bit longer, I really don't like some phrasing right now (and the title doesn't make sense anymore)

@chenglou
Copy link
Contributor Author

@vjeux alright, now it's acceptably short.

Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need [' ... ']. this.refs.item0.animate();

vjeux added a commit that referenced this pull request Dec 30, 2013
docs tips parent-child communication 2
@vjeux vjeux merged commit 3b0f705 into facebook:master Dec 30, 2013
@chenglou chenglou deleted the tips-communication branch December 30, 2013 22:12
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you noticed it, but you mutate this.state here. Perhaps one could prefer filtering instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though my preference is to keep the entry simple and not make newcomers wonder about too many things at the time. But I guess this isn't too much of an overhead.

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.

4 participants