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:
- 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 (
<div className="phase_box">
<h1>Phases</h1>
<PhaseList data={this.state.data} />
{ this.state.showForm ?
<PhaseForm onPhaseSubmit={this.handlePhaseSubmit} onHideFormLinkClick={this.handleHideFormLinkClick} /> :
<ShowFormLink onShowFormLinkClick={this.handleShowFormLinkClick} /> }
</div>
);
}
});
var PhaseList = React.createClass({
render: function() {
var phaseNodes = this.props.data.map(function (phase) {
return (
<Phase name={ phase.name } />
);
});
return (
<ul className="phase_list">
{ phaseNodes }
</ul>
);
}
});
var PhaseForm = React.createClass({
getInitialState: function() {
return { valid: true };
},
handleSubmit: function(e) {
e.preventDefault();
var nameInput = React.findDOMNode(this.refs.name);
var name = nameInput.value.trim();
if (!name) {
this.setState({ valid: false });
return;
}
this.props.onPhaseSubmit({ name: name });
nameInput.value = '';
return;
},
render: function() {
var nameInputClassName = this.state.valid ? null : 'has_errors';
return (
<form className="phase_form" onSubmit={this.handleSubmit}>
<input type="text" ref="name" className={nameInputClassName} />
<input type="submit" value="Create" />
<HideFormLink onHideFormLinkClick={this.props.onHideFormLinkClick} />
</form>
);
}
});
var Phase = React.createClass({
render: function() {
return (
<li className="phase">
{ this.props.name }
</li>
);
}
});
var ShowFormLink = React.createClass({
handleClick: function(e) {
e.preventDefault();
this.props.onShowFormLinkClick();
},
render: function() {
return (
<a href="#" onClick={this.handleClick}>New Phase</a>
);
}
});
var HideFormLink = React.createClass({
handleClick: function(e) {
e.preventDefault();
this.props.onHideFormLinkClick();
},
render: function() {
return (
<a href="#" onClick={this.handleClick}>Cancel</a>
);
}
});
data = [
{ "name": "Phase 1" },
{ "name": "Phase 2" },
{ "name": "Phase 3" }
];
React.render(<PhasesBox data={data} />, document.getElementById('container'));
1 Answer 1
I find your code pretty good mostly. Some ideas for possible improvements:
I don't think that
PhaseForm
should include theHideFormLink
sincePhaseForm
handles the state for that and the form shouldn't need to know that it might be hidden. I would rather move that up toPhasesBox
(eventually passing it as child down toPhaseForm
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
andHideFormLink
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 aLink
component that handles thepreventDefault
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 ingetInitialState
instead of usingcomponentDidMount
: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
incomponentDidMount
.
componentDidMount
, should it be{data: this.props.data}
? \$\endgroup\$