CODEWITHSUNDEEP
javascriptarraysreactjs
Asterix Assa
Asterix Assa

Reputation: 53

Pushing to array in React, unexpected behaviour

Trying to make a todo-list app

I want my todo items to have a unique key/id. I want to create the todo item id by taking the last todo item and adding 1 to its id. If the item does not exist it creates a new one with id = 1.

App.js:

import React, {Component} from 'react';
import Todos from './Todos'
import AddTodo from './AddForm'

class App extends Component {

  state = {
    todos: [
      {id: 1, content: 'buy milk'},
      {id: 2, content: 'play nintendo'}
    ]
  }

 addTodo = (todo) => {
    if (this.state.todos[0])
    {
      todo.id = this.state.todos[this.state.todos.length-1].id +1;
      let todos = [...this.state.todos];
      todos.push(todo);
      this.setState({
        todos: todos
      });
    }
    else
    {
      this.setState({
        todos: [
          {id: 1, content: todo.content}
        ]
      });
    }
    console.log(this.state.todos);
  }


  render() {
    return(
      <div className="todo-app container">
        <h1 className="center blue-text">Todos:</h1>
        <Todos deleteTodo={this.deleteTodo} todos={this.state.todos}/>
        <AddTodo addTodo={this.addTodo} />
      </div>
    );
  }
}

This almost works. I can add new items if the items have a unique content, but problems occur when the items have the same content.

Unique content console.log:

0: {content: "buy milk", id: 1}
1: {content: "play nintendo", id: 2}
2: {content: "adrfhahhafafdhafhafhhfa", id: 3}
3: {content: "adrfhahhafafdhafhafhhfaafhafhfah", id: 4}
4: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafh", id: 5}
5: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 6}

Same content console.log:

0: {content: "buy milk", id: 1}
1: {content: "play nintendo", id: 2}
2: {content: "adrfhahhafafdhafhafhhfa", id: 3}
3: {content: "adrfhahhafafdhafhafhhfaafhafhfah", id: 4}
4: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafh", id: 5}
5: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 7}
6: {content: "adrfhahhafafdhafhafhhfaafhafhfahafhfahafhafhafhfah", id: 7}

As you can see the id is the same if the content is the same.

I know that this is probably the wrong way to add id:s in react, but I want to understand what is going on. Thanks in advance.

EDIT: Yevgen pointed out a flaw with my logic if items are deleted, but I am still have not understood why the ID:s are repeating. Someone asked for the rest of the code so I added it all.

AddForm.js

import React, {Component} from 'react';

class AddTodo extends Component {
  state = {
    content: ''
  }

  handleChange = (e) => {
    this.setState({
      content: e.target.value
    });
  }

  handleSubmit = (e) => {
    e.preventDefault();
    this.props.addTodo(this.state);
  }

  render(){
    return (
    <div>
      <form onSubmit={this.handleSubmit}>
        <label>Add new todo</label>
        <input type="text" onChange={this.handleChange} />
      </form>
    </div>
    );
  }
}

export default AddTodo;

Todos.js

import React from 'react'

const Todos = ({todos, deleteTodo}) => {
  const todoList = todos.length ? (
    todos.map(todo => {
      return (
        <div className="collection-item" key={todo.id}>
          <span onClick={() => {deleteTodo(todo.id)}}>{todo.content}</span>
        </div>
      )
    })
  ) : (
    <p className="center">You have no todos left</p>
  )
  return (
    <div className="todos collection">
      {todoList}
    </div>
  )
}

export default Todos;

Upvotes: 1

Views: 169

Answers (2)

Yevhen Horbunkov
Yevhen Horbunkov

Reputation: 15530

My guess is that your issue comes from the way you declare <AddForm /> component's state. You should have done that within constrictor body, as this.state. What you do currently is global variable declaration. So, it isn't bound to your app state and your id is not getting assigned properly.

Other than that, your addTodo() looks way too overcomplicated and error prone (you can't rely on array length to get maximum used id, since deleting items, other than last will break the logic).

Besides, it's usually recommended to avoid using arrow notation for class methods. Instead, you should declare those as regular functions and do something, like this.addTodo = this.addTodo.bind(this) within constructor body for each method you declare. Otherwise, if you like arrow functions that much, you may enjoy those even more with function components.

Following your logic to assign autoincremented id's (thus, id's of deleted items never get reused) and passing object with property content to addTodo(), you could've simply done:

addTodo({content}){
   const id = Math.max(...this.state.todos.map(({id}) => id),0)+1
   this.setState({todos:[...this.state.todos, {id, content}]})
}

Upvotes: 3

Asterix Assa
Asterix Assa

Reputation: 53

I was able to get it working by fixing this:

 if (this.state.todos[0])
    {
      todo.id = this.state.todos[this.state.todos.length-1].id +1;
      let todos = [...this.state.todos];
      todos.push(todo);
      this.setState({
        todos: todos
      });
    }

With this:

 if (Array.isArray(this.state.todos) && this.state.todos.length)
    {
      todo.id = Math.max(...this.state.todos.map(({id}) => id),0)+1
      let todos = [...this.state.todos];
      todos.push({id: todo.id, content: todo.content})
      this.setState({
        todos: todos
      })
    }

Yevgen pointed out my issue with id names when deleting todo-list items. I fixed it. Thank you.

The repeating id names was somehow tied to the way i was pushing to the array... I still dont understand it fully, but it works now..

Upvotes: -1

Related Questions