Add "Handling Multiple Inputs" to Forms doc #8552

Merged
merged 3 commits into from Jan 18, 2017

Projects

None yet

5 participants

@keyanzhang
Member

It was brought up on this blog post (search for "Working with forms and Redux in React") and on Hacker News.

To be fair, this is a pretty common pattern of handling multiple inputs and it's not intuitive for someone to realize the name attribute can be used to achieve this. I think it's worth it to add it to the Forms section.

docs/docs/forms.md
+When you need to handle multiple controlled `input` elements, you can add a `name` attribute to each element and let a handler function choose what to do based on the value of `event.target.name`. For example:
+
+```javascript{14-15,18,32,38}
+class RSVP extends React.Component {
@gaearon
gaearon Jan 4, 2017 Member

Non-native speakers often have no idea what RSVP means.

docs/docs/forms.md
+ pendingState[event.target.name] = event.target.value;
+ break;
+ default:
+ // the code should never reach here
@gaearon
gaearon Jan 4, 2017 Member

Let's just cut this? It's annoying in the doc.

+ name="numberOfGuests"
+ type="number"
+ value={this.state.numberOfGuests}
+ onChange={this.handleInputChange}
@gaearon
gaearon Jan 4, 2017 Member

I think two inputs here means too much mental overhead. Let's just use one input for example?

+ );
+ }
+}
+```
@gaearon
gaearon Jan 4, 2017 Member

Please add a CodePen version.

@gaearon
Member
gaearon commented Jan 13, 2017

How do you feel about using the example in #8769 (comment) instead?

@gaearon gaearon self-assigned this Jan 13, 2017
@keyanzhang
Member

@gaearon sounds great to me! That's more clear than my example. Sorry, I've been traveling and didn't see your comments -- will update this PR by tomorrow.

@gaearon
Member
gaearon commented Jan 13, 2017

Thank you so much!

@keyanzhang
Member

Hey @gaearon, I revisited your example again carefully and I realized that our approaches are actually different. If I use your pattern to render a form with multiple input types, it quickly becomes more complex:

class MyComponent extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      name: 'Jane',
      surname: 'Smith',
      isGoing: false,
    };
  }

  handleChange(name, e) {
    if (name === 'isGoing') {
      this.setState({
        [name]: e.target.checked // a `checkbox` has `.checked` but not `.value`
      });
    } else {
      this.setState({
        [name]: e.target.value
      });      
    }
  }
  
  renderInput(name) {
    if (name === 'isGoing') {
      return (
        <input
          type="checkbox"
          onChange={e => this.handleChange(name, e)}
          value={this.state[name]}
        />        
      );
    }
    
    return (
      <input
      	type="text"
        onChange={e => this.handleChange(name, e)}
        value={this.state[name]}
      />
    );
  }

  render() {
    return (
      <div>
      	{this.renderInput('name')}
      	{this.renderInput('surname')}
      	{this.renderInput('isGoing')}
      </div>
    );
  }
}

It also creates more level of redirections (mental overhead) and closures (onChange={e => this.handleChange(name, e)}). I recall that when I was learning React I didn't realize that event.target references the real DOM object that dispatched the event; I thought event.target was a React only thing and didn't know there were more attributes (like event.target.name) other than event.target.value. This sounds stupid but it actually took me a while to figure out a better way to make complex forms without creating 10 functions/event handlers 😅. That's why I used the name attribute here.

How do you feel about using my example in the PR? I might be subjective on this and would love to have your feedback.

@gaearon
Member
gaearon commented Jan 15, 2017

OK, sounds good.

@keyanzhang keyanzhang renamed and added a codepen
1d17e82
docs/docs/forms.md
+
+ handleInputChange(event) {
+ const pendingState = {};
+ switch (event.target.name) {
@gaearon
gaearon Jan 17, 2017 Member

Why don’t we switch on the event target type instead?
I would prefer a more succinct version (since that's what the example tries to convey):

handleInputChange(e) {
  const value = e.target.type === 'checkbox' ? e.checked : e.value;
  const name = e.target.name;
  this.setState({
    [name]: value
  });
@keyanzhang
keyanzhang Jan 17, 2017 Member

Thanks, that's much clearer. :)

@keyanzhang keyanzhang simplified the example
865e5fa
@@ -186,7 +186,7 @@ Overall, this makes it so that `<input type="text">`, `<textarea>`, and `<select
When you need to handle multiple controlled `input` elements, you can add a `name` attribute to each element and let a handler function choose what to do based on the value of `event.target.name`. For example:
-```javascript{14-15,18,32,39}
@gaearon
gaearon Jan 17, 2017 Member

Codepen would need to update too?

@keyanzhang
keyanzhang Jan 18, 2017 Member

I've already updated the Codepen! Is there anything specific that needs improvement? :)

@hellonewbie
@gaearon gaearon merged commit 06399b8 into facebook:master Jan 18, 2017

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
coverage/coveralls Coverage increased (+1.2%) to 83.531%
Details
@gaearon
Member
gaearon commented Jan 18, 2017

Thanks!

@aweary aweary added a commit that referenced this pull request Jan 18, 2017
@keyanzhang @aweary keyanzhang + aweary Add "Handling Multiple Inputs" to Forms doc (#8552)
* added "Handling Multiple Inputs"

* renamed and added a codepen

* simplified the example

(cherry picked from commit 06399b8)
23ad42d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment