Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot Multiple Re-arrange on v11+ #1274

Closed
JakeCooper opened this issue Apr 25, 2019 · 10 comments · Fixed by #1284
Closed

Cannot Multiple Re-arrange on v11+ #1274

JakeCooper opened this issue Apr 25, 2019 · 10 comments · Fixed by #1284

Comments

@JakeCooper
Copy link

Expected behavior

You can rearrange as many times as you want.

Actual behavior

Some elements get stuck :(

Steps to reproduce

https://codesandbox.io/s/mymjv6o08j

Drag first element to the 3rd space.

Drag new first element anywhere. Can't drag it. Error in console

Suggested solution?

What version of React are you using?

React 16.8.6

What version of react-beautiful-dnd are you running?

v11.0.1

What browser are you using?

Chrome

Demo

https://codesandbox.io/s/mymjv6o08j

@alexreardon
Copy link
Collaborator

React is warning you in the console that your list does not have a key for each item in the list. If you add a key all is well. However, i could look into why rbd is crashing when this happens...

@bmueller-sykes
Copy link

bmueller-sykes commented Apr 26, 2019

So I'm seeing the same thing in my RBD-based Kanban, since upgrading to 11. Here's my codesandbox, which is a stripped-down version of my real code:

https://codesandbox.io/s/309ml3vlq5?fontsize=14

You should see three columns, each with 2 cards in them. You can sort the cards as much as you want--between columns, within columns, etc. But the column sort is problematic--if you sort the columns once (e.g. drag the second column to the first position), everything is fine. However, as soon as you start a second column drag, I see Invariant failed: cannot find critical draggable entry.

To my knowledge, I haven't changed anything of significance with my Kanban code in several months (and the handleDragEnd function hasn't changed in ages), and everything has worked. I only noticed the issue during QA testing earlier this week. Because this code is in the field, if the bug had been around for a while, I'd expect to have heard about it, so I think it's pretty recent.

@alexreardon
Copy link
Collaborator

Thanks @bmueller-sykes. I will take a look

@alexreardon
Copy link
Collaborator

You are using an index as a part of your key. Change it to this and everything is fine:

<div key={`kanban-column-${column.id}`}>

@alexreardon
Copy link
Collaborator

This can be an issue that people might bump into. Perhaps it is worth adding back a little improvement mentioned #1278 so at least people would not hit this issue

@alexreardon
Copy link
Collaborator

Using an index in the key is a violation of eslint react/no-array-index-key

@alexreardon
Copy link
Collaborator

@bmueller-sykes
Copy link

@alexreardon D'oh!! So...I'm somewhat confuse. This code has worked, unmodified, for me for something like 12 months--meaning, regardless of whether I got warnings in the console or not, I passed an index as part of the column key, and I was able to reorder columns to my heart's content. It sounds like you're not covering that mistake as fully in 11.x as you did in 10.x, but I suppose I would have still expected the column re-order to still break prior to 11.x, since (I'm assuming) you guys aren't in the business of fixing bad user key values. No?

Anyway, thanks for your attention here.

@alexreardon
Copy link
Collaborator

It should now have the old (still somewhat broken) behaviour. It will also log a nice warning to help you.

@bmueller-sykes
Copy link

I saw that, thanks. I went ahead and fixed my code so that it no longer uses indices, so I should be good going forward. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants