SortableJS / react-sortablejs

React bindings for SortableJS
http://sortablejs.github.io/react-sortablejs/
MIT License
2.03k stars 210 forks source link

I cannot update the list using this.setState() #174

Open andrepadeti opened 4 years ago

andrepadeti commented 4 years ago

I'm trying to build a 'put the words in order' typical language learning activity. The user is supposed to start by dragging the first word of a sentence to its right place and move on from there. Meanwhile, I'm checking whether the next word is in the right place. If so, that word/item changes CSS class and becomes not draggable and of a different background colour.

For this CSS class change to happen, I clone the draggable list of words, make the necessary changes and then I setState() with the new updated list. That's when I'm having problems. The list and the component will update occasionally, seems to be at random. I must be doing something wrong when I try to update the list of words in the handleDragEnd() method. Here's part of the code:

export default class Sentence extends Component {
  constructor(props) {
    super (props)

    // prepare this.state.list to be used by SortableJS
    let list = this.props.sentenceShuffled.map((word, index) => {
      return { id: (++index).toString(), name: word, class: 'sortable' }
    })

    this.state = {
      currentCorrectWord: 0, // stores how many words are correct already while student is sorting
      complete: false, // becomes true when sentence is sorted
      list: list // this object is used by SortableJS and is calculated in componentWillMount
    }

    this.handleDragEnd = this.handleDragEnd.bind(this)
  }

  handleDragEnd() {
    let currentCorrectWord = this.state.currentCorrectWord

    while ( this.state.list[currentCorrectWord].name === this.props.sentenceInOrder[currentCorrectWord]) {
      let newList = _.cloneDeep(this.state.list)
      newList[currentCorrectWord].class = 'notSortable'
      currentCorrectWord++

      /*
         this is where (I think) the problem is: setSate won't update states nor re-render component.
      */

      this.setState({ currentCorrectWord: currentCorrectWord })
      this.setState({ list: newList })

      ... more code here
    }
  }

  render() {
    return (
      ...more code here
            <ReactSortable
              list={this.state.list}
              setList={newState => this.setState({ list: newState })}
              filter=".notSortable"
            >
              {this.state.list.map(item => (
                <MDBBox 
                    className={item.class + ' eachWord'} 
                    key={item.id} 
                    onDragEnd={this.handleDragEnd} 
                    >{item.name}
                </MDBBox>
              ))}
            </ReactSortable>
      ...more code here
    )
  }
}

What am I doing wrong?

dagreatbrendino commented 3 years ago

I was able to throw this together in a code sandbox. It does a pretty decent job as far as I can tell. Part of the problem is that even if an option in your list is disabled, it can't be dragged, but it can still be moved if you drag another item past it. If you mess around with onChange, you might be able to work something out there to prevent that from happening.

Also, I'm pretty sure onDragEndshould just be onEnd, and should be a prop of <ReactSortable> and not it's children.

https://codesandbox.io/s/sortable-js-bug-forked-k3ih1?file=/src/App.js

andrepadeti commented 3 years ago

Thanks for your reply!

If I use let testList = [...this.state.list]; instead of let newList = _.cloneDeep(this.state.list) (we're using different variable names. lestList is the same as newList), this.state.list updates immediately after I newList[currentCorrectWord].class = 'notSortable' (in your version of the code, that would be testList[index].class = "notSortable";), even before I this.setState({ list: newList }), which makes me wonder if let testList = [...this.state.list]; is not simply copying the pointer instead of cloning the whole object.

On the other hand, if I let newList = _.cloneDeep(this.state.list), then change one of the elements in the array with newList[currentCorrectWord].class = 'notSortable' and then this.setState({ list: newList }), this.state.list won't necessarily update the list.

dagreatbrendino commented 3 years ago

No problem! I just learned something new thanks your reply...

So, using the spread operator like I did does copy the array and not the pointer. However, apparently the objects contained in the array only have their references copied and not the objects themselves.

I updated my sandbox so now it maps over the array and uses the spread operator on the objects of the array into the temp copy. I tested to make sure it wasn't modifying the objects until the setState this time.

Thanks again for teaching me something new! I've got a lot of code to update now 😓 https://codesandbox.io/s/sortable-js-bug-forked-k3ih1?file=/src/App.js