Changes to Forms doc #8084
Changes to Forms doc #8084
Conversation
| prev: state-and-lifecycle.html | ||
| next: lifting-state-up.html | ||
| redirect_from: "/tips/controlled-input-null-value.html" | ||
| prev: |
There was a problem hiding this comment.
Why were these removed? Every page in the "Basic Guides" has this links.
Compare Lists and Keys (has these links):
and Forms after this change (doesn't have these links anymore, why?):
| --- | ||
|
|
||
| ## Forms |
There was a problem hiding this comment.
I intentionally removed this subheader in the past.
If you add it, this is how the page looks:
The double heading is unnecessary. If you look at other pages, none of the guides have double headings.
| @@ -61,7 +62,7 @@ class Form extends React.Component { | |||
| ReactDOM.render(<Form />, document.getElementById('root')); | |||
| ``` | |||
|
|
|||
| [Try it on CodePen.](https://codepen.io/ericnakagawa/pen/QKkJva?editors=0010) | |||
| [Try it on Codepen](https://codepen.io/ericnakagawa/pen/QKkJva?editors=0010) | |||
There was a problem hiding this comment.
I intentionally changed it to "CodePen" the last time because this is how the service is capitalized:
What is the reason for changing it back?
| @@ -114,8 +115,6 @@ class Form extends React.Component { | |||
| return ( | |||
| <div> | |||
| <input type="text" | |||
| placeholder="Hello!" | |||
There was a problem hiding this comment.
Why do we remove the placeholder too? It is not related to controlled-ness of the components.
| @@ -126,7 +125,7 @@ class Form extends React.Component { | |||
| ReactDOM.render(<Form />, document.getElementById('root')); | |||
| ``` | |||
|
|
|||
| [Try it on CodePen.](https://codepen.io/ericnakagawa/pen/XjxyoL?editors=0010) | |||
| [Try it on Codepen](https://codepen.io/ericnakagawa/pen/XjxyoL?editors=0010) | |||
There was a problem hiding this comment.
Same here: I intentionally changed it to CodePen before.
| @@ -389,7 +390,7 @@ class Form extends React.Component { | |||
|
|
|||
| handleChange(event) { | |||
| let val = event.target.value; | |||
There was a problem hiding this comment.
Both of these can be const, not let.
| @@ -389,7 +390,7 @@ class Form extends React.Component { | |||
|
|
|||
| handleChange(event) { | |||
| let val = event.target.value; | |||
| let checked = this.state.checked.slice(); // copy | |||
| let checked = this.state.checked.slice(); // copy | |||
| if(checked.includes(val)) { | |||
There was a problem hiding this comment.
Please put space between if and the opening brace.
Please don't use includes(), it's a ES2016 API that is unsupported by any other than most recent browsers.
| @@ -114,8 +115,6 @@ class Form extends React.Component { | |||
| return ( | |||
| <div> | |||
| <input type="text" | |||
| placeholder="Hello!" | |||
| value={this.state.value} | |||
There was a problem hiding this comment.
Can you please add line highlighting for the relevant lines in these examples? They're pretty long, and it would help to highlight whatever parts or properties that are being explained.
|
|
||
| ### Basic Select | ||
| ### Basic Uncontrolled Select |
There was a problem hiding this comment.
Isn't it controlled? The example is setting value on select.
| <input type="radio" name="choice" value="A" onChange={this.handleChange} /> Option A<br /> | ||
| <input type="radio" name="choice" value="B" onChange={this.handleChange} defaultChecked={true} /> Option B<br /> | ||
| <input type="radio" name="choice" value="C" onChange={this.handleChange} /> Option C<br /> | ||
| <input type="radio" name="choice" value="A" |
There was a problem hiding this comment.
This is not very readable. There is also a problem: clicking on the label doesn't select the option. This is not a very good UX.
I also don't think name attribute is relevant in this example.
We should use <label> instead, like this:
<label>
<input type="radio" value="A" onChange={this.handleChange} />
Option A
</label>
|
On a broader scale, I'm not sure why we have controlled examples for input, textarea, and select, but uncontrolled radio and checkbox. Can we show a controlled and uncontrolled example for each of them? I also feel that this document is too long and has too many details compared to other "basics" guide. If I was learning React I would stop reading somewhere in the middle, and miss the next important topics. Can we split this guide into "Controlling Inputs" and "Forms in Depth" or something like this? Also, can you please move the events section to the events reference as we discussed? |
|
Did something weird happen while merging? This pull request looks like it is undoing a bunch of stuff that it doesn't seem like you intended to undo. Also you should target the facebook:master branch rather than facebook:new-docs now that new-docs is merged. |
|
Applied changes in this PR: #8112 |
|
@gaearon I like the idea of lightening up this document. I think it would be good to break this out in a future pass. |
Fixes 8052
Fixes 8068