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

[WIP] Update scroll ancestor detection when the draggable is not over a droppable #54

Closed
wants to merge 3 commits into from

Conversation

clauderic
Copy link
Owner

@clauderic clauderic commented Jan 13, 2021

This PR is currently a work in progress, but will eventually resolve #43.

Currently, if the active draggable is not over a droppable, the scroll ancestor logic assumes that the scroll ancestors should be those of the active draggable.

This behaviour is problematic for a number of reasons, including those outlined in #43.

An alternative approach that I'm exploring in this PR is to find the scroll element automatically based on the center point of the active draggable. This isn't a perfect solution, which is why I'm opening this PR as a draft PR for the time being.

This solution isn't perfect currently because the point coordinates used for scroll ancestor detection should be affected by modifiers, but the modifiers also need the scroll ancestors. One solution for this that I'm currently thinking of would be to distinguish between the scroll target ancestors and the active node scroll ancestors.

Also, the document.getElementFromPoint method is expensive, and therefore calls to it should be debounced to avoid performance issues. Finding a scroll ancestor when the draggable is not over a droppable is something that can be done asynchronously and doesn't necessarily need to happen immediately for every drag move event.

@changeset-bot
Copy link

changeset-bot bot commented Jan 13, 2021

⚠️ No Changeset found

Latest commit: 5da5f52

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

@github-actions
Copy link
Contributor

Size Change: +424 B (+1%)

Total Size: 62.9 kB

Filename Size Change
packages/core/dist/core.cjs.development.js 16.6 kB +181 B (+1%)
packages/core/dist/core.cjs.production.min.js 10.1 kB +71 B (+1%)
packages/core/dist/core.esm.js 16.4 kB +172 B (+1%)
ℹ️ View Unchanged
Filename Size Change
packages/accessibility/dist/accessibility.cjs.development.js 979 B 0 B
packages/accessibility/dist/accessibility.cjs.production.min.js 637 B 0 B
packages/accessibility/dist/accessibility.esm.js 847 B 0 B
packages/accessibility/dist/index.js 148 B 0 B
packages/core/dist/index.js 141 B 0 B
packages/modifiers/dist/index.js 144 B 0 B
packages/modifiers/dist/modifiers.cjs.development.js 838 B 0 B
packages/modifiers/dist/modifiers.cjs.production.min.js 574 B 0 B
packages/modifiers/dist/modifiers.esm.js 766 B 0 B
packages/sortable/dist/index.js 144 B 0 B
packages/sortable/dist/sortable.cjs.development.js 3.96 kB 0 B
packages/sortable/dist/sortable.cjs.production.min.js 2.53 kB 0 B
packages/sortable/dist/sortable.esm.js 3.85 kB 0 B
packages/utilities/dist/index.js 144 B 0 B
packages/utilities/dist/utilities.cjs.development.js 1.63 kB 0 B
packages/utilities/dist/utilities.cjs.production.min.js 962 B 0 B
packages/utilities/dist/utilities.esm.js 1.55 kB 0 B

compressed-size-action

@py-wai
Copy link
Contributor

py-wai commented Apr 29, 2021

Hi @clauderic 👋

Love your library, huge work!

This PR concerns the Sortable preset as well?

@clauderic
Copy link
Owner Author

Hey @py-wai, the sortable preset is built on top of @dnd-kit/core and thus benefits from any updates/fixes made to the core lib

@arpithindukuri
Copy link

@clauderic thank you for the fantastic library and great support.

I have faced similar issues to #43, and I am happy to try and help solve this.

I am slowly learning the inner workings of this library, so is there any way I can help with this PR?

@Bnaya Bnaya mentioned this pull request Jul 12, 2021
@clauderic
Copy link
Owner Author

Closing in favor of #518

@clauderic clauderic closed this Nov 23, 2021
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.

RectIntersection is not working with draggable items inside a scrollable container
3 participants