patternjavascriptMinor
ReactJS app to add and validate items to a list
Viewed 0 times
validatereactjsappitemsandlistadd
Problem
I wrote a sample app based on the ReactJS tutorial, and I would like some thoughts on how idiomatic this code is. Unfortunately there's no ReactJS integration with Stackoverflow snippets, but I do have a working JSFiddle.
You have a phases box, and inside it we have a list of phases, and a new phase form to add a phase to the list. There's a link to show the form, and another link on the form to hide it. There's also validation on the form.
I'm curious to know how idiomatic this is. Some things to consider:
```
var PhasesBox = React.createClass({
getInitialState: function() {
return {
data: [],
showForm: false
};
},
componentDidMount: function() {
this.setState({ data: data });
},
handlePhaseSubmit: function(phase) {
var phases = this.state.data;
var newPhases = phases.concat([phase]);
this.setState({ data: newPhases, showForm: false });
},
handleShowFormLinkClick: function(arg) {
this.setState({ showForm: true });
},
handleHideFormLinkClick: function(arg) {
this.setState({ showForm: false });
},
render: function() {
return (
Phases
{ this.state.showForm ?
:
}
);
}
});
var PhaseList = React.createClass({
render: function() {
var phaseNodes = this.props.data.map(function (phase) {
return (
);
});
return (
{ phaseNodes }
);
}
});
var PhaseForm = React.createClass({
You have a phases box, and inside it we have a list of phases, and a new phase form to add a phase to the list. There's a link to show the form, and another link on the form to hide it. There's also validation on the form.
I'm curious to know how idiomatic this is. Some things to consider:
- I'm passing a callback to handle hiding the from from the parent to the form, and then to the link component that hides the form. Seems excessive to pass down a callback through descendents.
- Validation is very complex, but I have a simple case here: text is required. It seems a bit ad hoc to validate this way.
```
var PhasesBox = React.createClass({
getInitialState: function() {
return {
data: [],
showForm: false
};
},
componentDidMount: function() {
this.setState({ data: data });
},
handlePhaseSubmit: function(phase) {
var phases = this.state.data;
var newPhases = phases.concat([phase]);
this.setState({ data: newPhases, showForm: false });
},
handleShowFormLinkClick: function(arg) {
this.setState({ showForm: true });
},
handleHideFormLinkClick: function(arg) {
this.setState({ showForm: false });
},
render: function() {
return (
Phases
{ this.state.showForm ?
:
}
);
}
});
var PhaseList = React.createClass({
render: function() {
var phaseNodes = this.props.data.map(function (phase) {
return (
);
});
return (
{ phaseNodes }
);
}
});
var PhaseForm = React.createClass({
Solution
I find your code pretty good mostly. Some ideas for possible improvements:
-
I don't think that
-
I would inline
-
Same for
-
-
You can initialize the state of
In addition to being shorter, it also prevents a re-rendering triggered because of using
-
I don't think that
PhaseForm should include the HideFormLink since PhaseForm handles the state for that and the form shouldn't need to know that it might be hidden. I would rather move that up to PhasesBox (eventually passing it as child down to PhaseForm if you need to have this exact DOM tree). With this, there would also be no need to pass the callback down anymore.-
I would inline
ShowFormLink and HideFormLink since both do nearly nothing (and nearly the same). Also, you wouldn't need to pass down the callback anymore. If you like it more, you can create a Link component that handles the preventDefault on its click handler.-
Same for
Phase (IMHO)-
React.findDOMNode(this.refs.name) is no longer needed, this.refs.name already gives you the DOM node (at least in more recent React versions)-
You can initialize the state of
PhaseBox with the passed down data directly in getInitialState instead of using componentDidMount:getInitialState: function() {
return {
data: this.props.data || [], // or using `getDefaultProps()` if you prefer that
showForm: false
};
}In addition to being shorter, it also prevents a re-rendering triggered because of using
setState in componentDidMount.Code Snippets
getInitialState: function() {
return {
data: this.props.data || [], // or using `getDefaultProps()` if you prefer that
showForm: false
};
}Context
StackExchange Code Review Q#84573, answer score: 2
Revisions (0)
No revisions yet.