patternjavascriptrailsMinor
React.js components for a golf app
Viewed 0 times
golfappreactcomponentsfor
Problem
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
GolfCourse:
Hole:
Yardage:
I load the component using:
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
Tee box select component
```
var TeeBoxSelect = React.createClass({
getInitialState: funct
I'm using the
react-rails gem, and my data structure is as follows:GolfCourse:
has_many :holesHole:
belongs_to :golf_course
has_many :yardagesYardage:
belongs_to :tee_boxI load the component using:
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 (
{option.number}
{option.name}
{typeof(yardage) !== "undefined" ? yardage.yards : ""}
{typeof(yardage) !== "undefined" ? yardage.par : ""}
{typeof(yardage) !== "undefined" ? yardage.stroke_index: ""}
);
});
},
render: function() {
return (
Hole
Yards
Par
Stroke Index
Score
{this.renderHoles()}
);
}
});Tee box select component
```
var TeeBoxSelect = React.createClass({
getInitialState: funct
Solution
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...
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
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.
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.
Code Snippets
this.setState({
yardage: this.props.yardages.find(yardage => yardage.hole_id === option.id) || {};
});Context
StackExchange Code Review Q#108985, answer score: 2
Revisions (0)
No revisions yet.