onClick event only leaves the last item of the DOM array instead of removing the target

Admin 1
Viewed 653
class ToDo extends React.Component {
  constructor() {
    super()
    this.state = {
      todos: [],
      inputValue: null
    }
    this.changeValue = this.changeValue.bind(this)
    this.addTodo = this.addTodo.bind(this)
    this.removeTodo = this.removeTodo.bind(this)
  }

  changeValue(e) {
    this.setState({inputValue: e.target.value})
  }

  addTodo(e) {
    e.preventDefault()

    const newTodoItem = {
      id: Date.now(),
      inputValue: this.state.inputValue
    }

    if(!this.state.inputValue.length) return

    this.setState(state => ({
      todos: state.todos.concat(newTodoItem),
      inputValue: ''
    }))
  }

  removeTodo(e) {
    const et = e.target
    const todos = this.state.todos

    this.setState({
      todos: todos.splice(todos.indexOf(et), 1)
    })
  }

  render() {
    return(
      <div>
        <form onSubmit={this.addTodo}>
          <input
            type='text'
            name='todo'
            placeholder='Enter a ToDo'
            value={this.state.inputValue}
            onChange={this.changeValue}
          />
          <input type='submit' value='Add' />
          &nbsp;&nbsp;&nbsp;
          <span>ToDos: {this.state.todos.length}</span>
        </form>
        <TodoList items={this.state.todos} itemClickEvent={this.removeTodo} />
      </div>
    )
  }
}

class TodoList extends React.Component {
  render() {
    const items = this.props.items
    return(
      <ul>
        {
          items.map(item => (
            <li key={item.id} onClick={this.props.itemClickEvent}>{item.inputValue}</li>
          ))
        }
      </ul>
    )
  }
}

ReactDOM.render(<ToDo />, document.querySelector('#app'))

In this todo list sample that I just imitated from the sample in the reactjs.org homepage, I added an event that removes the targeted todo item when clicked. The problem is, whichever of the todo items I click, instead of removing the target item itself, it removes all the items except the last item. Why such bug is happening?

reactjs target arrays javascript
2 Answers

You should to make a few changes to make this work properly:

  • Use filter instead of splice, as filter returns a new array as opposed to modifying the existing array. It's best practice to avoid modifying the existing state.
  • Instead of grabbing todos before setState, use the variation of setState that passes the current state as the first parameter to a function in setState.
  • Add a unique identifier to the li items that you can reference in your removeTodo function. Right now, todos.splice(todos.indexOf(et), 1); will not return anything meaningful, as et is an Element, and todos is not an array of Elements -- it is an array of Objects that have two keys: id and inputValue. What you can do instead is add a data-key attribute to your li that equals the key attribute, and then reference data-key in your removeTodo.

When you put it all together, this is the change you make to the JSX returned by the TodoList class:

items.map(item => (
    <li key={item.id} data-key={item.id} onClick={this.props.itemClickEvent}>{item.inputValue}</li>
))

Notice that we set data-key equal to item.id.

Our modified removeTodo will look like this:

removeTodo(e) {
    const et = e.target
    const attr = et.getAttribute("data-key");

    this.setState(state => ({
      todos: state.todos.filter(item => item.id != attr)
    }))
}

I've modified your JSFiddle here with a working example.

Edit:

Although the solution above works just fine, there's an alternative solution that is shorter and avoids having to add a data-key attribute to the inputs:

Pass the item.id directly to removeTodo by making this change:

items.map(item => (
    <li key={item.id} onClick={() => this.props.itemClickEvent(item.id)}>{item.inputValue}</li>
))

Then, simplify your removeTodo to work by using its id parameter:

removeTodo(id) {
    this.setState(state => ({
      todos: state.todos.filter(item => item.id != id)
    }))
 }

JSFiddle here with the new solution.

user_50065880

Here we go, Try this:

removeTodo(e) {
    const et = e.target
    const todos = this.state.todos
    todos.splice(todos.indexOf(et), 1);
    this.setState({
      todos
    })
}

splice() return:

An array containing the deleted elements. If only one element is removed, an array of one element is returned. If no elements are removed, an empty array is returned.

user_23578480
Related