3
\$\begingroup\$

I'm quite new to React and was working through the "Thinking in React" example, I noticed they used refs for updating state, but I also read on SO that using refs isn't that good in the long run, so I tried to redo this without using refs:

JSFiddle

var SearchBar = React.createClass({
    handleChange: function(event) {
        if (event.target.type ==='text') {
            this.props.onUserInput(
                event.target.value,
                this.props.inStockOnly
            );
        } else {
            this.props.onUserInput(
                this.props.filterText,
                event.target.checked
            );
        }
    },

    render: function() {
        return (
            <form>
                <input 
                    type="text" 
                    placeholder="Search..." 
                    value={this.props.filterText} 
                    onChange={this.handleChange}
                />
                <p>
                    <input 
                        type="checkbox" 
                        checked={this.props.inStockOnly}
                        onChange={this.handleChange}
                    />
                    {' '}
                    Only show products in stock
                </p>
            </form>
        );
    }
});

It works but it seems repetitive, how would someone comfortable with React do this without refs?

\$\endgroup\$
  • 2
    \$\begingroup\$ Please edit your title, and your post, to tell us a bit more about what your code is doing. The title in particular, should at least hint at the purpose of your code. \$\endgroup\$ – Mathieu Guindon Dec 24 '15 at 2:28
  • \$\begingroup\$ Welcome to Code Review! Good job on your first question. \$\endgroup\$ – SirPython Dec 24 '15 at 2:44
2
\$\begingroup\$

Actually, I'd worry more about the issue that's bound to happen - your handleChange trying to do everything. I suggest you split them off to two distinct functions, one for the text input, and the other for the checkbox. This is much more maintainable in the long run, as each function is only responsible for one task. This ain't mixed martial arts :P

There's nothing wrong with refs. They're simply for easy access to the DOM elements inside the component. What's probably wrong is when an parent component accesses child refs, or a component exposing its refs to other components. Components should only receive props and state, and emit events with data (like a regular DOM element). You don't mess with how it implements its internals.

Anyways, not much change here:

var SearchBar = React.createClass({
  handleInputChange: function(){
    this.props.onUserInput(this.refs.myInput.value, this.props.inStockOnly);
  },
  handleCheckboxChange: function(){
    this.props.onUserInput(this.props.filterText, this.refs.myCheckbox.value);
  },
  render: function() {
    return (
      <form>
        <input type="text" placeholder="Search..." value={this.props.filterText} onChange={this.handleInputChange} ref="myInput" />
        <div>
          <label>
            <input type="checkbox" checked={this.props.inStockOnly} onChange={this.handleCheckboxChange} ref="myCheckbox" id="myCheckbox" />
            <span>Only show products in stock</span>
          </label>
        </div>
      </form>
    );
  }
});

Now regarding your markup, <p> means paragraph but you have an <input>. I believe <p> was simply used for layouting. <div> is more appropriate. It also appears that your text is a label for the input. I suggest you wrap them in a <label> so clicking the label toggles the box. An additional <span> is recommended since you might want to style the text differently from a text-only label.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.