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

Use controlled components in tutorial #5445

Merged
merged 1 commit into from
Nov 18, 2015
Merged

Use controlled components in tutorial #5445

merged 1 commit into from
Nov 18, 2015

Conversation

yangshun
Copy link
Contributor

Updated tutorial and use controlled components instead of refs, as mentioned in #5443. @zpao

<input type="text" placeholder="Your name" ref="author" />
<input type="text" placeholder="Say something..." ref="text" />
<input type="text" placeholder="Your name"
value={this.state.author} onChange={this.handleAuthorChange}/>
Copy link
Member

Choose a reason for hiding this comment

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

I know this is a lot of lines but its our style and I think we should use it so we're consistent with the rest of our code samples.

<input
  type="text"
  placeholder="Your name"
  value={this.state.author}
  onChange={this.handleChange}
/>

@zpao
Copy link
Member

zpao commented Nov 11, 2015

Thanks for taking this on!

This is a lot of code to introduce all at once - I think we might need a bit more in the paragraph preceding that first big code change to tie it together more.

Let's make the form interactive. When the user submits the form, we should clear it, submit a request to the server, and refresh the list of comments. To start, let's listen for the form's submit event and clear it.

It's technically accurate - that is exactly what we're doing. But then you see a whole lot of code changing to do something that on the surface seems pretty straightforward. So I think saying something about the fact that we're going to use state to store the user input as it is entered - another use of state that we didn't discuss above when we talked about state. We're also definitely doing more than "listen for the form's submit event and clear it".

@facebook-github-bot
Copy link

@yangshun updated the pull request.

@yangshun
Copy link
Contributor Author

@zpao Great suggstions! I have broken the code changes down into smaller and more digestable chunks. Have a read of the modified version at http://yangshun.im/react/docs/tutorial.html#adding-new-comments

@yangshun
Copy link
Contributor Author

@zpao what do you think?


```javascript{3-14,16-19}
```javascript{3-11,14-26}
Copy link
Member

Choose a reason for hiding this comment

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

Could you make this 27 (to catch the closing </form> tag (it's already messed up on the site so not your fault)

@zpao
Copy link
Member

zpao commented Nov 18, 2015

Otherwise, this looks really awesome, thanks! Could you fix that one bit and squash the commits, then we can merge this. Would you like to update the tutorial repo as well (https://github.com/reactjs/react-tutorial/blob/master/public/scripts/example.js)? I'm happy to do that but figured I'd give you the chance if you wanted :)

@yangshun
Copy link
Contributor Author

Yes I'd gladly make the fixes for the tutorial repo too! Gimme a moment to modify the highlighting and submit a PR to the tutorial repo (:

@yangshun
Copy link
Contributor Author

@zpao Do I have to update the tutorials of the other languages too?

@facebook-github-bot
Copy link

@yangshun updated the pull request.

@zpao
Copy link
Member

zpao commented Nov 18, 2015

No, let's let the translators do that. Unless you happen to know Italian, Japanese, Korean, and Chinese…

@yangshun
Copy link
Contributor Author

But they will have to sync up the code changes too since another snippet has been added. Will that be fine?

@zpao
Copy link
Member

zpao commented Nov 18, 2015

Yea, that'll be fine. We don't do a good job of surfacing those translations so it's not a huge deal.

@zpao
Copy link
Member

zpao commented Nov 18, 2015

Awesome, let's get these in and I'll update the site :)

zpao added a commit that referenced this pull request Nov 18, 2015
@zpao zpao merged commit 904e9e3 into facebook:master Nov 18, 2015
@yangshun
Copy link
Contributor Author

Got it! The PR for the react-tutorial can be found here: reactjs/react-tutorial#101

zpao added a commit that referenced this pull request Nov 18, 2015
Use controlled components in tutorial
(cherry picked from commit 904e9e3)
@yangshun yangshun deleted the controlled-components-in-tutorial branch November 19, 2015 03:26
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

3 participants