6
\$\begingroup\$

I've recently started to look into React. I feel like I'm understanding it, but I want to make sure I'm doing the best practices before I make too many bad habits.

I'm using the react-rails gem, and my data structure is as follows:

GolfCourse:

has_many :holes

Hole:

belongs_to :golf_course
has_many :yardages

Yardage:

belongs_to :tee_box

I load the component using:

<%= react_component('Scorecard', teeBoxes: TeeBox.all, holes: @course.holes, yardages: @yardages) %>

The idea behind the component is: The user has loaded the page to enter their recent golfing score, the page loads the scorecard for the current course they have selected. The user then changes the dropdown to select which tee box they played off.

The scorecard yardage/par/stroke_index will change depending on which tee colour they have chosen.


Scorecard Component

var Scorecard = React.createClass({

  getInitialState: function() {
    return {
      yardages: {}
    };
  },

  changeYardage: function(teeBoxId) {
    this.setState({
      yardages: _.filter(this.props.yardages, function(yardage){ return yardage.tee_box_id == teeBoxId; })
    });
  },

  renderHoles: function() {
    var state = this.state;
    return this.props.holes.map(function(option) {
      yardage = _.find(state.yardages, function(yardage) { return yardage.hole_id == option.id; });

      return (
        <tr className="hole" key={option.id}>
          <td>{option.number}</td>
          <td>{option.name}</td>
          <td>{typeof(yardage) !== "undefined" ? yardage.yards : ""}</td>
          <td>{typeof(yardage) !== "undefined" ? yardage.par : ""}</td>
          <td>{typeof(yardage) !== "undefined" ? yardage.stroke_index: ""}</td>
          <td></td>
        </tr>
      );
    });
  },

  render: function() {
    return (
      <div className="scorecard">
        <TeeBoxSelect teeBoxes={this.props.teeBoxes} changeYardage={this.changeYardage} />

        <table className="scorecard--holes">
          <thead>
            <tr>
              <th colSpan="2">Hole</th>
              <th>Yards</th>
              <th>Par</th>
              <th>Stroke Index</th>
              <th>Score</th>
            </tr>
          </thead>
          <tbody>
            {this.renderHoles()}
          </tbody>
        </table>
      </div>
    );
  }
});

Tee box select component

var TeeBoxSelect = React.createClass({
  getInitialState: function() {
    return {
      selected: 0
    }
  },

  handleChange: function() {
    this.props.changeYardage(this.refs.teeBox.value);
  },

  optionNodes: function() {
    return this.props.teeBoxes.map(function(option) {
      return <option key={option.id} value={option.id}>{option.colour}</option>;
    });
  },

  render: function() {
    return (
      <select ref="teeBox" value={this.state.selected} onChange={this.handleChange}>
        <option value="0">-- Select your tee box --</option>
        {this.optionNodes()}
      </select>
    );
  }
});

How did I do? My main concerns are where I'm rendering the multiple elements (holes <tr> in scorecard component, tee box options in the teebox component.), and the way I handle state (yardages) in the Scorecard component.

\$\endgroup\$
2
\$\begingroup\$

Your component is pretty good. I even had to look at this guide to check when props and state should be used, and your code pretty much nailed it. There are a few things to change though.

First is to not rely on the fact that props and state get handed down to your component. It's best if you provide default props the same way you did with state. That way, you're always sure they have values. It also serves as pseudo-documentation, telling the developer that's lost and found his way into your component that this component expects the following props and state.

So... changeYardage selects a yardage, a single yardage. It makes no sense keeping it an array and end up using find later on. You can use find on changeYardage instead so that yardages stores an object instead of an array. If find returns undefined, just default it to a blank object. And since it's a single yardage object, we rename it to a singular yardage instead.

this.setState({
  yardage: this.props.yardages.find(yardage => yardage.hole_id === option.id) || {};
});

I notice you use React, so probably you're using Babel (if not, you should). This gives you access to a lot of shortcuts seen above. First, in ES6, arrays now have a find method so you can drop using lodash/underscore for that. Then there's the arrow functions to make callbacks a bit shorter. Not related to ES6 syntax, you should also use strict comparison, unless the comparison calls for loose comparison.

Now with regards to the yardages table, I'm not sure you want to actually show a blank table when yardage is not defined. You might want to actually just show an error message instead of a blank table.

And I notice you use BEM convention for CSS. Good. At least you are not using inline styles, because it has a very high specificity and makes it hard for stylesheets to override.

\$\endgroup\$
  • \$\begingroup\$ thanks, I'll take this into consideration, specifically the yardage/default props stuff :). Nice to see i'm doing "ok"! A bit concern of mine was typeof(yardage) !== "undefined" ? yardage.yards : "" if state wasnt yet set, is this generally how you'd handle this inr eact? \$\endgroup\$ – Samuel Cambridge Oct 28 '15 at 13:52
  • 1
    \$\begingroup\$ @SamuelCambridge Not sure if there's a convention for it, but that looks fine to me. You could also offload that check into a function within the component to avoid repeating the check logic. \$\endgroup\$ – Joseph Oct 28 '15 at 14:02

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.