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 nested list stories #1524

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

Conversation

chrisvxd
Copy link

@chrisvxd chrisvxd commented Nov 10, 2024

Description

This PR:

  1. adds nested list stories to reproduce a bug causing items to be positioned incorrectly when dragging between nested droppables. You can reproduce this bug by dragging an item out of the list and into the root.
  2. fixes a bug where droppables can collide with their own droppable children

Nested issues

  1. Possible to collide with the droppable child of the currently dragged droppable (fixed by bd0c918, addressed in user-land by Puck)
  2. General unstableness when dragging in-and-out of nested droppables (addressed in user-land by Puck)
  3. Feedback plugin is spooked in this story with keyboard sensor
  4. Dragging item outside of list causes a resize, but the calculation to offset this is now wrong. This is due to the reversal of the width / initialSize.width logic to account for iframes (fixed in
    cd400b1)

Other issues

  1. Keyboard plugin doesn't work when dragging items out of iframes

Copy link

changeset-bot bot commented Nov 10, 2024

🦋 Changeset detected

Latest commit: d111206

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 7 packages
Name Type
@dnd-kit/dom Patch
@dnd-kit/react Patch
@dnd-kit/abstract Patch
@dnd-kit/collision Patch
@dnd-kit/geometry Patch
@dnd-kit/helpers Patch
@dnd-kit/state Patch

Not sure what this means? Click here to learn what changesets are.

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

@chrisvxd chrisvxd marked this pull request as ready for review November 10, 2024 17:23
}
}

// Filter out collisions of items that contain other items
Copy link
Author

@chrisvxd chrisvxd Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment could be clarified

Suggested change
// Filter out collisions of items that contain other items
// Filter out collisions of droppable children

@@ -90,4 +95,6 @@ export class Droppable<
public get isDropTarget() {
return this.manager?.dragOperation.target?.id === this.id;
}

public path: UniqueIdentifier[] = [];
Copy link
Owner

@clauderic clauderic Nov 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this something like:

  @reactive
  public accessor parent: UniqueIdentifier | undefined;

  @derived
  public get path() {
    const {droppables} = manager.registry;
    const {parent} = this;
    const path = [];
    let parent = droppables.get(parent);
   
    while (parent) {
        path.push(parent.id);
        parent = droppables.get(parent.parent);
    }
    
    return path;
  }

And then the @dnd-kit/dom implementation can override path and automatically infer the path based on the DOM if this.parent is undefined

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it more abstract in d111206, but chose to override parent instead of path in the @dnd-kit/dom implementation so that .parent will still work. Let me know what you think.

x: coordinatesDelta.x / frameTransform.scaleX - sizeDelta.width,
y: coordinatesDelta.y / frameTransform.scaleY - sizeDelta.height,
x: coordinatesDelta.x / frameTransform.scaleX + sizeDelta.width,
y: coordinatesDelta.y / frameTransform.scaleY + sizeDelta.height,
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to rework this logic to account for regular size changes, too.

It seems to be mostly working, but keyboard sensor positions item a few px out, which has me suspicious

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.

2 participants