5
\$\begingroup\$

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:

  1. 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.
  2. 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'));
\$\endgroup\$
  • 3
    \$\begingroup\$ Before anyone who is unfamiliar with ReactJS gets really confused as to why this code works, it is JSX and not Javascript, which is why OP can put HTML literals inside of his code. JSX does compile into JavaScript and is visually similar to JavaScript so I am leaving the JavaScript tag on. @Mohamad grats on being the first person to have a question in the JSX tag. \$\endgroup\$ – Dan Pantry Mar 20 '15 at 14:30
  • \$\begingroup\$ @DanPantry Cheers for sorting out the question, and shame there's no reward the latter achievement! More can be read on JSX \$\endgroup\$ – Mohamad Mar 20 '15 at 20:03
  • 1
    \$\begingroup\$ Within your componentDidMount, should it be {data: this.props.data}? \$\endgroup\$ – Adrian Lynch May 7 '16 at 0:03
  • \$\begingroup\$ It's nice that you're handling validation within your PhaseForm. I started doing that too at the start. Have a read up on controlled form fields: blog.iansinnott.com/… - Including the comments. It's an interesting read about handling forms in React. \$\endgroup\$ – Adrian Lynch May 7 '16 at 0:21
2
\$\begingroup\$

I find your code pretty good mostly. Some ideas for possible improvements:

  • 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.

\$\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.