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

Update Lifting State Up not to mix up DOM value with component state #9032

Merged
merged 5 commits into from
Mar 2, 2017

Conversation

valscion
Copy link
Contributor

@valscion valscion commented Feb 20, 2017

A few weeks ago when teaching my friend, she got stuck on this.state.value vs. event.target.value. As the documentation talked a lot about "values", and the term "value" could mean three different things (values in general, the value prop / DOM value of the <input> component and the value in state/props), it was not weird that she got a bit confused.

I asked about this already from @gaearon in Twitter here: https://twitter.com/valscion/status/833776451442843649

A few weeks ago when teaching my friend, she got stuck on
`this.state.value` vs. `event.target.value`. As the documentation
talked a lot about "values", and the term value could mean three
different things (values in general, the "value" prop / DOM value of
the <input> component and the value in state/props), it was not weird
that she got a bit confused.
@gaearon
Copy link
Collaborator

gaearon commented Feb 20, 2017

Can we also rename onChange then?

@valscion
Copy link
Contributor Author

Yeah, we could. Maybe we could rename it to onTemperatureChange? Would that be OK?

This is in-line with how the temperature is provided as a prop named `temperature`
@valscion
Copy link
Contributor Author

There, that should do it ☺️. Let me know if you want further adjustments — I'm able to look at this again tomorrow

@valscion
Copy link
Contributor Author

Anything I could do to get this shipped? Do I need to update the code pens to contain the new code, too?

@valscion
Copy link
Contributor Author

Let me know if it was OK to change the codepen examples to also contain the updated code. Feel free to fork the pens for your account if you don't want them to show up under my name ☺️

@gaearon
Copy link
Collaborator

gaearon commented Feb 24, 2017

Can you also make a new GIF at the end? 😅

@valscion
Copy link
Contributor Author

valscion commented Feb 26, 2017

Oh yeah, sure, I can do that 😄. Funny how I didn't see how many things would need to be changed but I'm totally fine with this 😄

@valscion
Copy link
Contributor Author

valscion commented Mar 1, 2017

That should do it ☺️. Is there a way I could add the review-needed label back myself?

@gaearon
Copy link
Collaborator

gaearon commented Mar 1, 2017

We are no longer using our custom labels. 😉

@gaearon gaearon merged commit a190cfc into facebook:master Mar 2, 2017
@gaearon
Copy link
Collaborator

gaearon commented Mar 2, 2017

OK, that’s pretty great. Thanks for doing this.

@valscion valscion deleted the patch-1 branch March 2, 2017 08:00
@valscion
Copy link
Contributor Author

valscion commented Mar 2, 2017

🎉 Thanks for merging!

@valscion
Copy link
Contributor Author

valscion commented Mar 9, 2017

Hiya,

how and when do you update the documentations normally? It's been a week since this PR was merged. Do you only update the website with new major versions or some other milestone?

@gaearon
Copy link
Collaborator

gaearon commented Mar 9, 2017

Not really, just when somebody runs a script that updates the docs.

gaearon pushed a commit that referenced this pull request Mar 9, 2017
…9032)

* Update Lifting State Up not to mix up DOM value with component state

A few weeks ago when teaching my friend, she got stuck on
`this.state.value` vs. `event.target.value`. As the documentation
talked a lot about "values", and the term value could mean three
different things (values in general, the "value" prop / DOM value of
the <input> component and the value in state/props), it was not weird
that she got a bit confused.

* Rename Lifting State Up onChange props to onTemperatureChange

This is in-line with how the temperature is provided as a prop named `temperature`

* Fix one value prop not being renamed to temperature

* Update codepen examples in Lifting state up documentation

* Update devtools state change to reflect docs change

(cherry picked from commit a190cfc)
@gaearon
Copy link
Collaborator

gaearon commented Mar 9, 2017

Ran the script. Docs should update soon.

@gaearon
Copy link
Collaborator

gaearon commented Mar 10, 2017

Updated.

@valscion
Copy link
Contributor Author

Thank you, I noticed yesterday already ☺️

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.

4 participants