To-Do List in React

I've written a to-do list app in React, as I am a beginner and this is my first app, I feel it can be improved. Some assistance in improving it, and how or why improvements are better would be much appreciated.

Here is a working demo of it.

```
var ToDoContainer = React.createClass({
getInitialState: function(){
var tasks = [
{task:"Go to the gym", completed:false, id:1 },
{task:"Do yoga", completed:false, id:2 },
{task:"Buy groceries", completed:true, id:3 },
{task:"Get tire fixed", completed:true, id:4}
];

return {
tasks:tasks,
numCompleted:null
}
},
addTask: function(task){
this.setState({
tasks:this.state.tasks.concat(task)
});
},
clearComplete: function(){

var remainingTasks = this.state.tasks.filter(function(task){
if (!task.completed) return task;
});

this.setState({
tasks:remainingTasks
});

},
markComplete: function(e){
console.log('markComplete')
var id = e.target.getAttribute('data-id');
var tempTasks = this.state.tasks;

tempTasks.forEach(function(task){
if (id == task.id) {
if (task.completed){
task.completed = false;
}
else if (!task.completed) {
task.completed = true;
}
}
})

this.setState({
tasks:tempTasks
});
},
render: function(){
var numCompleted = this.state.tasks.filter(function(task){
return task.completed;
}).length

var numRemaining = this.state.tasks.filter(function(task){
return !task.completed;
}).length

return (


You have {numRemaining} tasks to do:

return {
    tasks:tasks,
    numCompleted:null
}

numCompleted isn't being used anywhere. Remove if from the data. Also, it's a "computed property", one you can compute off from tasks. I notice you compute it on render. Consider putting it in a function instead.

getCompletedTaskCount: function(){
  return this.state.tasks.filter(task => task.completed).length;
},
getNotCompletedTaskCount: function(){
  return this.state.tasks.filter(task => !task.completed).length;
},
clearComplete: function(){
    this.setState({
        tasks: this.state.tasks.filter(task => !task.completed);
    });
},

Since you're using React, I assume you use Babel, and thus we have access to ES6 transpilation. ES6 provides new syntax to simplify your code. In the example above, I use arrow functions and their ability to implicitly return the expression value.

Also, I notice that you count the complete and incomplete tasks on render. I suggest you move these logic into functions so that they can be reused. It also makes them visible, and reduces clutter in the render function.

markComplete: function(e){
    console.log('markComplete')
    var id = e.target.getAttribute('data-id');
    var tempTasks = this.state.tasks;

    tempTasks.forEach(function(task){
        if (id == task.id) {
            if (task.completed){
                task.completed = false; 
            }
            else if (!task.completed) {
                task.completed = true;  
            }
        }
    })

    this.setState({
        tasks:tempTasks
    });
},

I suggest against console.log for debugging. The last thing you'd ever want to make a habit of is forgetting console calls in your code. I suggest you learn using breakpoints in dev tools. Much more powerful and doesn't leave trash in the code.

markComplete is misleading. It's not really just marking the tasks complete. It's toggling the complete state. You should name it appropriately, like toggleCompleteness.

return 

The problem here is your CSS classes. complete and incomplete are very generic. It could possibly be killed by other libraries in your app that use those very same names. With that, you'd start to think about namespacing, and your CSS tends to end up like:

.todo-list .task .complete{...}

The effect is that this style has a specificity of 0-3-0 (0 ids, 3 classes, 0 elements). To override it, you'll need 1-x-x (an id) or 0-4-0 (4 classes) or a 0-3-0 (3 classes, but defined after). This becomes a headache as the app grows larger, and make your CSS life a battle of overrides.

To counter this problem, I suggest you read about BEM. In a gist, it's a naming convention for CSS classes that allow you to build low-specificity, precisely targeted selectors that are easily identifiable. You don't even need to override if you can safely say this style only affects this component and nothing else.

this.props.addNew({task:this.state.newTask, completed:false, id:Date.now()});

The completed and id should be filled in by the addTask in the very top-most component, and that addNew should only be called with newTask. This is to isolate the knowledge of the data structure to just one location in the code.

A relevant scenario would be when your data structure changes. If you wrote it this way, you'd be hunting for hard-coded data structures. Whereas if you just isolated it in one place, you'd only change that portion of the code.

Also, don't use Date.now() as an ID. Generate GUIDs instead. Here's an handy implementation in JS. If you're just after a quick way to identify things, I personally use ('' + Math.random()).slice(2) for a quick ID solution as Date.now() can yield the same value if you run it in rapid succession (like in loops).

A ` is overkill here, as your handler doesn't use anything about the form aside from hooking to the onSubmit event. You can remove the , replace the input button input with a and put an onClick on it that calls handleSubmit.

var buttonStyle = {
    marginTop: "10px"
}

Another reason to use BEM convention is that inline styles are higher in specificity than IDs, and the only way to override them is to replace the inline style or use !important` in a stylesheet. Again, you don't want your CSS to be a battle of overrides.

StackExchange Code Review Q#116181, answer score: 6