2
\$\begingroup\$

Would this todo list benefit from breaking it down into more components? What would be the advantages? Am I breaking any best practices from what is seen here?

var TodoList = React.createClass({
    getInitialState: function(){
        return {
            todos:data,
            text:""
        };
    },
    handleChange: function(event){
        this.setState({text:event.target.value });
    },
    submitHandler: function(event){
        event.preventDefault();
        var todo = this.state.text;
        data.push({task:todo});
        this.setState({text:""});
        $('#input').val("")
    },
    render: function(){
        var rows = [];
        this.state.todos.forEach( function(todo, index){
            rows.push(<li key={index}>{todo.task}</li>)
        }); 

        return(
            <div className="well clearfix">
                <form onSubmit={this.submitHandler}>
                    <ul>
                        {rows}
                    </ul>
                    <br />
                    <input id="input" key={this.state.timestamp} onChange={this.handleChange} type="text" />
                    <br />
                    <br />
                    <button type="submit" className="btn btn-primary" disabled={this.state.text.length === 0}>Submit</button>
                </form>
            </div>
        )
    }
});

var data = [
    {task:"eat cheese"},
    {task:"go to gym"},
    {task:"do some yoga"},
    {task:"go to sleep"}
];

ReactDOM.render(<TodoList/>, document.getElementById('app') );
\$\endgroup\$
1
\$\begingroup\$

I think data should just be contained in the component state, because it belongs to the TodoList:

getInitialState: function() {
    return {
        todos: [{task: "eat cheese"}, ...],
        text: ""
    };
},

which would simplify submitHandler into

submitHandler: function(event) {
    event.preventDefault();
    var todo = this.state.text;
    this.setState({
        text: "",
        todos: this.state.todos.concat([todo])
    });
},

The jQuery $('#input').val("") call is unnecessary because React should be handling that state for us. Just make the value of the <input> reflect state.text and all will flow:

<input id="input" key={this.state.timestamp} onChange={this.handleChange}
       value={this.state.text} type="text" />

In render(), the declaration of rows and the forEach can be combined into a map:

var rows = this.state.todos.map(function(todo, index) {
    return (<li key={index}>{todo.task}</li>);
});

--

In general your code is very clear. Right now your app is simple and I don't think there's much need to break it down into smaller parts.

However, as your app inevitably grows and you add more features, separating, say, TodoItem and TodoList into distinct components would be useful. When a function gets long and starts doing five or six conceptually distinct things, you separate it into smaller functions and compose them together. Same thing when classes start representing many things at once.

So some advantages of breaking things down into smaller components are modularity and composability. Your app becomes easier to test and think about.

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