[Sortable] Incorrect layout animation on drop when using the `rectSwappingStrategy`

[valtism]

Hi there 👋

First of all, wanted to say a huge thanks for this library. It's incredibly easy to use, works super well and is super polished. You should be very proud of it.

Anyway, the issue I'm facing is that with a sortable grid, when I drag an item to any position, except for its direct neighbour to the left or right (interestingly, even if the neighbour is on a different row), there will be a double animation fired of the drop (swap) target moving from it's original position to it's new position. I would not expect this, instead I would expect that since it is already there it would not have to move again.

You can see this happening here:

https://user-images.githubusercontent.com/1286001/122676117-d7b95a80-d21f-11eb-8f79-279783a8d9ae.mov

And this is a sandbox to play with this behaviour:

https://codesandbox.io/s/zealous-bell-lfng3?file=/src/App.js

I think this may be something that I have done wrong rather than a bug in the library, but I've been tearing my hair out and I can't seem to track down what is wrong here. I would really appreciate some help solving this.

[valtism]

Ok, after some more fiddling with this, I figured out that I can use animateLayoutChanges: () => false in useSortable.

I still find this confusing, because in the storybook examples, the <SortableItem> components have animateLayoutChanges: undefined, which I believe should set them back to the default animation. Despite this, they seem to handle drop animations fine.

Going to leave this issue open until I can get some clarity from the dev.

[clauderic]

Hey @valtism, thanks for the kind words.

The main difference between your example and the storybook examples is that you are using rectSwappingStrategy. I'll have to spend more time investigating into why the layout animation is yielding this behaviour when used in coordination with rectSwappingStrategy.

For now as you have discovered, you can get around the issue by disabling layout changes in useSortable

[valtism]

This doesn't just happen with rectSwappingStrategy, you can change it to rectSortingStrategy in the codeSandbox and see that you still get this layout animation happening.

[clauderic]

@valtism that's because of the way you're moving items on sort end. Your custom sort function assumes that the items need to swap position instead of being sorted. If you replace it with arrayMove that is exported from @dnd-kit/sortable you won't see that behavior.

[individual11]

I was able to replicate this error using the verticalListSortingStrategy and the arrayMove function fired within the onDragEnd handler.

The animateLayoutChanges solution works for me as well, but wanted to bring it to light as I was researching this issue.

Also agree this library is absolutely epic!

[joeflem]

We are also running into this problem when trying to set up a grid, example: https://codesandbox.io/s/react-dnd-kit-swapping-test-p9mue

In the example, if you drag diagonally the expected animations appear on the grid, the items swap over diagonally. However once you finish dragging it's almost like the arrayMove is using rectSortingStrategy by default.

https://user-images.githubusercontent.com/45002285/138487875-6555c68a-de55-41b0-9f0e-b8e07d59780c.mov

@valtism The solution above doesn't seem to work for us 😢 Any chance you could share a sandbox with your fix?

[clauderic]

There were a couple things that weren't quite set up properly with the sandbox you posted above @ciavuc, mainly:

• You were using components that use useSortable within <DragOverlay>, which is not permitted
• You were not rendering the <DragOverlay> when the activeIndex was 0 because zero is falsy (activeIndex ? <DragOverlay /> : null vs activeIndex != null ? <DragOverlay /> : null)

See https://codesandbox.io/s/react-dnd-kit-swapping-test-forked-imkxc?file=/src/Grid/Grid.jsx

[joeflem]

There were a couple things that weren't quite set up properly with the sandbox you posted above @ciavuc, mainly:

• You were using components that use useSortable within <DragOverlay>, which is not permitted
• You were not rendering the <DragOverlay> when the activeIndex was 0 because zero is falsy (activeIndex ? <DragOverlay /> : null vs activeIndex != null ? <DragOverlay /> : null)

See https://codesandbox.io/s/react-dnd-kit-swapping-test-forked-imkxc?file=/src/Grid/Grid.jsx

Thanks for pointing those out. Much appreciated!

Edit: I think my original issue still remains, if you shrink down so you have two columns, dragging tile 4 to tile 1 move tile 1 to tile 2, although I'm still unsure whether it's a genuine problem or something we've done wrong during set up. Upon looking again, It may be a different issue than the one above - I'll open a discussion regarding it.

[clauderic]

@ciavuc the problem is that you're using arrayMove on drop, which does not re-order the array how you would need to when implementing swappable functionality.

For example, if your array is ['A', 'B', 'C', 'D', E'] and you want to move A to D's position, arrayMove will generate the following new array: ['B', 'C', 'D', 'A', 'E']. This makes sense when you're sorting items, but not when you're looking to swap items.

To swap items, you need to use a different method than arrayMove. You can author a fairly simple arraySwap helper:

function arraySwap(items, from, to) {
   const newItems = items.slice();

   newItems[from] = items[to];
   newItems[to] = items[from];

  return newItems;
}

const items = ['A', 'B', 'C', 'D', 'E'];

const newItems = arraySwap(items, 0, 3); // ['D', 'B', 'C', 'A', 'E']

As of the next release of @dnd-kit/sortable (5.1.0), an arraySwap helper will be provided by default which you'll be able to import.

You can start using this by installing the versions of @dnd-kit/sortable tagged with next on npm:

I also pushed a fix in #486 that fixes an issue with the layout animation in useSortable on drop. By default, useSortable assumes that most consumers will be using arrayMove to compute the newIndex of an item on drop. This isn't always the case however, so I added the getNewIndex prop on useSortable, which you can now use to compute the new index an item will have on drop.

In this example, that logic looks like:

function getNewIndex({id, items, activeIndex, overIndex}) {
  return arraySwap(items, overIndex, activeIndex).indexOf(id);
}

The only difference between this and the default getNewIndex method is the use of arraySwap instead of arrayMove (refer to https://github.com/clauderic/dnd-kit/blob/master/packages/sortable/src/hooks/defaults.ts#L11-L16)

I've implemented all these changes here: https://codesandbox.io/s/react-dnd-kit-swapping-test-forked-imkxc

See also https://5fc05e08a4a65d0021ae0bf2-ejxxqkfwok.chromatic.com/?path=/story/presets-sortable-grid--swappable

I believe this fixes all issues specific to supporting swappable functionality that were described here, though feel free to re-open new issues if you find other issues.

[joeflem]

I believe this fixes all issues specific to supporting swappable functionality that were described here, though feel free to re-open new issues if you find other issues.

@clauderic this is amazing, fixes all issues we had with this package.

Thanks again for the package itself and the time spent on this specific issue! 👍