adding onClick functionality to React to do list

Question

I've created a to do list with React. I want to be able to mark tasks completed simply by clicking on them. I also want to be able to clear task that have been completed by clicking a button. These two functions are not working correctly with the code I have set up. When I click on an individual todo item to mark it complete, every single to do item on the list gets marked complete and thus, appears with a 'line-through.' When I then click the designated button to clear completed tasks, absolutely nothing happens. Can someone help me resolve these two issues?

code from App component:

class App extends React.Component {
  constructor() {
    super();
    this.state = {
      todos: [
        {
          task: "learn how to fly drone",
          id: Date.now(),
          completed: false
        }, 
        {
          task: "learn React class components",
          id: Date.now(),
          completed: false
        },
        {
          task: "practice editing videos",
          id: Date.now(),
          completed: false
        },
        {
          task: "read Ten Years A Nomad",
          id: Date.now(),
          completed: false
        }
    ],
      todo: ''
    }
  }

  inputChangeHandler = event => {
    this.setState({[event.target.name]: event.target.value})
  }

  addTask = event => {
    event.preventDefault();
    let newTask = {
      task: this.state.todo,
      id: Date.now(),
      completed: false
    };
    this.setState({
      todos: [...this.state.todos, newTask],
      todo: ''
    })
  }

  toggleComplete = itemId => {
    const todos = this.state.todos.map(todo => {
      if (todo.id === itemId) {
        todo.completed = !todo.completed
      }
      return todo
    });
    this.setState({todos, todo: ''})
  }

  clearCompleted = e => {
    e.preventDefault();
    return this.state.todos.filter(item => !item.completed)
  }

  render() {
    return (
      <div className="App">
        <h2>Welcome to your Todo App!</h2>
        <TodoList 
          todos={this.state.todos} 
          toggleComplete={this.toggleComplete} />
        <TodoForm todos={this.state.todos} value={this.state.todo} inputChangeHandler={this.inputChangeHandler} addTask={this.addTask} clearCompleted={this.clearCompleted}/>
      </div>
    );
  }
}

export default App;

TodoList:

const TodoList = props => {
    return (
        <div>
            {props.todos.map((todo, id) => (
                <Todo 
                    todo={todo} 
                    key={id} 
                    toggleComplete={props.toggleComplete} />
            ))}
        </div>
    )
}

export default TodoList;

Todo:

const Todo = props => {
    return (
        <div 
            key={props.todo.id}
            onClick={() => {
                props.toggleComplete(props.todo.id)
            }}>
            <p 
                style={{textDecoration: props.todo.completed ? 'line-through' : 'none'}}>
                {props.todo.task}
            </p>
        </div>
    )
}

export default Todo;

TodoForm:

const TodoForm = props => {
    return (
        <form>
            <input 
                name="todo" 
                value={props.value} 
                type="text" 
                onChange={props.inputChangeHandler} 
                placeholder="Enter new task" />
            <button onClick={props.addTask}>Add Todo</button>
            <button onClick={props.clearCompleted}>Clear Completed</button>
        </form>
    )
}

export default TodoForm;

wentjun · Accepted Answer · 2019-12-27 02:57:05Z

5

1) The reason why every item is marked is because all objects within the todos state has the same id. Therefore, toggleComplete() will end up marking all objects within todos as true.

What you can do is to assign each object with a unique id, instead of assigning all id with the same Date object.

Here is an example:

constructor() {
  super();
  this.state = {
    todos: [
      {
        task: "learn how to fly drone",
        id: 1,
        completed: false
      }, 
      {
        task: "learn React class components",
        id: 2,
        completed: false
      },
      {
        task: "practice editing videos",
        id: 3,
        completed: false
      },
      {
        task: "read Ten Years A Nomad",
        id: 4,
        completed: false
      }
  ],  
    todo: ''
  }
}

2) Next, clearCompleted is not calling setState(), hence, none of the tasks are cleared. I assume that you are trying to set completed as false? In that case, you can simply do set completed as false for all objects, and then update your state.

clearCompleted = e => {
  e.preventDefault();
  const todos = this.state.todos.map(todo => ({
    ...todo,
    completed: false,
  }));

  this.setState({
    todos,
  });
}

I have created a demo which fixes your issues.

edited Dec 27, 2019 at 2:57

answered Dec 27, 2019 at 2:44

wentjun

42.8k10 gold badges108 silver badges116 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

Matt Oestreich Over a year ago

Nice answer! I rushed my answer and overlooked that.. so obvious now... this should be selected as the accepted answer.

wentjun Over a year ago

@MattOestreich glad to help! Feel free to check out the demo I have created. The problems should be solved :)

Jevon Cochran Over a year ago

Thanks. I was able to fix the issues based on your answer. The clearCompleted function that you have up in the demo doesn't work just FYI. I got it to work by doing this... clearCompleted = e => { e.preventDefault(); const todos = this.state.todos.filter(item => !item.completed); this.setState({ ...this.state, todos }) }

Matt Oestreich · Accepted Answer · 2019-12-27 03:17:51Z

Edit:

@wentjun's answer should be selected as the accepted answer.. I am going to leave this answer up though, as I still feel it provides value. To elaborate: I prefer to pass the index as it's a little faster then having to map over every single todo item just to find the one that was clicked. (Even if you used this.state.todos.find(itm => itm.id === itemId, passing the index is faster since you already know which item was clicked..

Original Answer:

I modified the ToDo component to pass the entire todo object in the click event, as well as passing the todo item index as well as the todo item within the ToDoList component. This way you can grab the index of the todo item that was clicked, to easily change the completed property on that specific todo item within state.

Even though I am not doing anything with the todo item object that is being passed via the click event, I recommend still passing the entire object, just in case - makes things more flexible. Open up your console to see the todo object that gets clicked.

Edit: I updated the clearCompleted to:

  clearCompleted = e => {
    e.preventDefault();
    let stateCopy = {...this.state};
    stateCopy.todos = stateCopy.todos.reduce((acc, cur) => 
      [...acc, {...cur, completed: false}], []
    );
    this.setState(stateCopy);
  };

I also modified your setState call within the click handler.. It is best practice to always make a copy of your state, modify the copy, then update state with the copy.

Demo:

class App extends React.Component {
  constructor() {
    super();
    this.state = {
      todos: [
        {
          task: "learn how to fly drone",
          id: Date.now(),
          completed: false
        },
        {
          task: "learn React class components",
          id: Date.now(),
          completed: false
        },
        {
          task: "practice editing videos",
          id: Date.now(),
          completed: false
        },
        {
          task: "read Ten Years A Nomad",
          id: Date.now(),
          completed: false
        }
      ],
      todo: ""
    };
  }

  inputChangeHandler = event => {
    this.setState({ [event.target.name]: event.target.value });
  };

  addTask = event => {
    event.preventDefault();
    let newTask = {
      task: this.state.todo,
      id: Date.now(),
      completed: false
    };
    this.setState({
      todos: [...this.state.todos, newTask],
      todo: ""
    });
  };

  toggleComplete = (todoItem, todoItemIndex) => {
    let stateCopy = { ...this.state };
    let item = stateCopy.todos[todoItemIndex];
    item.completed = !item.completed;
    this.setState(stateCopy, () => 
      console.log(this.state.todos[todoItemIndex])
    );
  };

  clearCompleted = e => {
    e.preventDefault();
    let stateCopy = {...this.state};
    stateCopy.todos = stateCopy.todos.reduce((acc, cur) => 
      [...acc, {...cur, completed: false}], []
    );
    this.setState(stateCopy);
  };

  render() {
    return (
      <div className="App">
        <h2>Welcome to your Todo App!</h2>
        <TodoList
          todos={this.state.todos}
          toggleComplete={this.toggleComplete}
        />
        <TodoForm
          todos={this.state.todos}
          value={this.state.todo}
          inputChangeHandler={this.inputChangeHandler}
          addTask={this.addTask}
          clearCompleted={this.clearCompleted}
        />
      </div>
    );
  }
}

const TodoList = ({ todos, toggleComplete }) => {
  return (
    <div>
      {todos && todos.map((todo, index) => (
        <Todo
          todo={todo} 
          key={index} 
          toggleComplete={() => toggleComplete(todo, index)} /* <<--- Pass the item and index to the handler function */ 
        />                                                   /* Even though we are not using the actual todo item     */
                                                             /* object, still not a bad idea to pass it thru          */ 
        
      ))}
    </div>
  );
};

const Todo = props => {
  return (
    <div 
      key={props.todo.id} 
      onClick={() => { props.toggleComplete(props.todo) }}> {/* Pass entire todo object, just in case you need it */}
      <p 
        style={{ 
          cursor: 'pointer',
          textDecoration: props.todo.completed ? "line-through" : "none" 
        }}>
        {props.todo.task}
      </p>
    </div>
  );
};

const TodoForm = props => {
  return (
    <form>
      <input
        name="todo"
        value={props.value}
        type="text"
        onChange={props.inputChangeHandler}
        placeholder="Enter new task"
      />
      <button onClick={props.addTask}>Add Todo</button>
      <button onClick={props.clearCompleted}>Clear Completed</button>
    </form>
  );
};


ReactDOM.render(<App />, document.body);

<script src="https://cdnjs.cloudflare.com/ajax/libs/react/16.12.0/umd/react.production.min.js"></script>
<script src="https://cdnjs.cloudflare.com/ajax/libs/react-dom/16.11.0/umd/react-dom.production.min.js"></script>

Collectives™ on Stack Overflow

adding onClick functionality to React to do list

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related