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

Add verticalSortableList collision detection strategy #805

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ceddlyburge
Copy link

@ceddlyburge ceddlyburge commented Jun 16, 2022

Fixes #804

Probably this will want a little demo or something added, if you are happy with it, maybe in Stories?

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2022

⚠️ No Changeset found

Latest commit: cef8e5e

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@marcospassos
Copy link

I spent a few hours trying to understand the weird behavior we were expecting here until I finally found this issue. I can confirm that this collision strategy makes vertical lists that don't use drag overlay work properly.

It would be really helpful to have this as a building collision strategy to avoid this boilerplate code everywhere.

@ceddlyburge
Copy link
Author

One thing to note is that it doesn't work with autoscrolling (scrolling the container while dragging). We have been using this code with the autoscrolling option turned off for a while now, and it's been working fine, so I think it's solid.

@TheMikeyRoss
Copy link

any update on this one? could it possibly be related to an issue that looks like this
dnd-kit-variable-height

@ceddlyburge
Copy link
Author

You could copy the code in to your project and see if it fixes the problem?

@TheMikeyRoss
Copy link

TheMikeyRoss commented Nov 8, 2023

Thanks @ceddlyburge for your reply,

May I ask how do I use verticalSortableListCollisionDetection in my code?

@ceddlyburge
Copy link
Author

Its surprisingly easy!

Just copy all the code in this PR somewhere, and then pass the function in to the DndContext.

<DndContext
      collisionDetection={verticalSortableListCollisionDetection}
      autoScroll={false}
      ...
>

@TheMikeyRoss
Copy link

Awesome, thanks @ceddlyburge 👍

@TheMikeyRoss
Copy link

@ceddlyburge It works wonderfully! I really appreciate it ♥
Here's a demo below 👇

Before

dnd-kit-variable-height-before

After

dnd-kit-variable-height-after

@ceddlyburge
Copy link
Author

ceddlyburge commented Nov 9, 2023

No worries, glad you like it :)
Also, nice demo :)

@ozdemircibaris
Copy link

ozdemircibaris commented Nov 23, 2023

@ceddlyburge
Hello Cedd,
I have a similar problem too. Actually, the only difference is that my structure is horizontal. I refactored your code according to horizontal style, but it did not work as I wanted. Can you help me too?

dnd.bug.mov
import { CollisionDetection, DroppableContainer } from '@dnd-kit/core';
import { sortBy } from 'lodash';

export const horizontalSortableListCollisionDetection: CollisionDetection = (
  args
) => {
  if (args.collisionRect.left < (args.active.rect.current?.initial?.left ?? 0)) {
    return leftmostDroppableContainerMajorityCovered(args);
  } else {
    return rightmostDroppableContainerMajorityCovered(args);
  }
};

const leftmostDroppableContainerMajorityCovered: CollisionDetection = ({
  droppableContainers,
  collisionRect,
}) => {
  const ascendingDroppableContainers = sortBy(
    droppableContainers,
    (c) => c?.rect.current?.left
  );

  for (const droppableContainer of ascendingDroppableContainers) {
    const {
      rect: { current: droppableRect },
    } = droppableContainer;

    if (droppableRect) {
      const coveredPercentage =
        (droppableRect.left + droppableRect.width - collisionRect.left) /
        droppableRect.width;

      if (coveredPercentage > 0.5) {
        return [collision(droppableContainer)];
      }
    }
  }

  return [collision(ascendingDroppableContainers[0])];
};

const rightmostDroppableContainerMajorityCovered: CollisionDetection = ({
  droppableContainers,
  collisionRect,
}) => {
  const descendingDroppableContainers = sortBy(
    droppableContainers,
    (c) => c?.rect.current?.left
  ).reverse();

  for (const droppableContainer of descendingDroppableContainers) {
    const {
      rect: { current: droppableRect },
    } = droppableContainer;

    if (droppableRect) {
      const coveredPercentage =
        (collisionRect.right - droppableRect.left) / droppableRect.width;

      if (coveredPercentage > 0.5) {
        return [collision(droppableContainer)];
      }
    }
  }

  return [collision(descendingDroppableContainers[0])];
};

const collision = (droppableContainer?: DroppableContainer) => {
  return {
    id: droppableContainer?.id ?? '',
    value: droppableContainer,
  };
};

@ceddlyburge
Copy link
Author

Hi Baris, I'll take a look when I get some time. I can't remember all the details of this, but hopefully it will come back to me fairly quickly :)

@ozdemircibaris
Copy link

I am waiting for your return, thank you very much

@ceddlyburge
Copy link
Author

HI Baris, this looks like it should work to me, do you have some example code I can debug?
Thanks, Cedd

@ozdemircibaris
Copy link

Hi Cedd, I will prepare it quickly and share the link
Thanks

@ozdemircibaris
Copy link

Hey Cedd,
Sorry for the late reply, I don't have any problems right now, my above code works.

@ceddlyburge
Copy link
Author

Ah great, glad to hear it!
Maybe you could add the horizontalSortableListCollisionDetection to this PR?

@ozdemircibaris
Copy link

Yes of course!

@ceddlyburge
Copy link
Author

I've added horizontalSortableListCollisionDetection to this PR now, hopefully it will be useful for someone in the future.

@TheMikeyRoss
Copy link

Amazon work @ceddlyburge 👍

@clauderic could this be merged? or is it gonna have some potential conflict with your planned refactoring mentioned [here] ?(#1194 (comment))

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

Successfully merging this pull request may close these issues.

useSortable with variable size items and without a drag overlay
4 participants