-
-
Notifications
You must be signed in to change notification settings - Fork 595
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
Autoscrolling improvements #140
Conversation
🦋 Changeset detectedLatest commit: cc13c98 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
Size Change: +1.94 kB (+2%) Total Size: 84.5 kB
ℹ️ View Unchanged
|
@clauderic I want to try this branch out in my project that uses dnd-kit, but not sure how to point to it, instead of the installed npm |
You can try using This PR is still a bit of a work in progress, the acceleration and thresholds need a bit of polish. Also considering letting consumers pass in their own autoscroller offsets |
|
||
useEffect(() => { | ||
if (disabled || !scrollableAncestors.length || !draggingRect) { | ||
if (disabled || !scrollableAncestors.length || !pointerCoordinates) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know if it's intended, but it looks possible that sortScrollableAncestors
could remove elements. Does it make sense the check the sorted ancestors list instead?
if (disabled || !scrollableAncestors.length || !pointerCoordinates) { | |
if (disabled || !sortedScrollableAncestors.length || !pointerCoordinates) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I realized that could be an unintended consequence of giving consumers a method as flexible as sortedScrollableAncestors
. I've since refactored this approach to only give consumers the option decide whether to set the scroll order to the tree order or reversed tree order 800b2d6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you accept a sort function instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semi related to this change... is (or will) there be a way to stop an element scrolling?
When I first saw this PR I was thinking I could use it to stop the window scrolling horizontally, which is an issue I'm facing now with a vertical-only scrolling list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if you accept a sort function instead?
I'd prefer to limit the use-cases here for now, can't really think of good use-cases where you would want to scroll ancestors in an order that doesn't match the DOM (reversed or otherwise).
Semi related to this change... is (or will) there be a way to stop an element scrolling?
I hadn't considered that use-case, but that could certainly be added.
Scroll ancestors are necessary in a few places in the library for collision detection to work properly, so I'd prefer to only let consumers decide if a given ancestor can be scrolled.
Something like:
// canScroll(scrollAncestor: Element): boolean;
<DndContext autoScroll={{
enabled: true,
order: TraversalOrder.ReversedTreeOrder,
canScroll(element) {
if (element === document.scrollingElement) {
return false;
}
return true;
}
}} />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me and I agree on the sorting 👍
d2feb26
to
18bcbae
Compare
…tScrollableAncestors function
…erval and acceleration
18bcbae
to
4aed3fe
Compare
68b3c4e
to
20f2475
Compare
20f2475
to
cc13c98
Compare
Hello, I had issues with autoScroll.thresholds after hours and hours debugging I found out that it is autoScroll.threshold, without s.
|
@andelkocvjetkovic Thank you. You saved my sanity. @clauderic Maybe you could consider editing your first comment above? |
Fixes #23
Auto-scrolling defaults have been updated, which should generally lead to improved user experience for most consumers.
The auto-scroller now bases its calculations based on the position of the pointer rather than the edges of the draggable element's rect by default. This change is aligned with how the native HTML 5 Drag & Drop auto-scrolling behaves.
This behaviour can be customized using the
activator
option of theautoScroll
prop:The auto-scroller now also looks at scrollable ancestors in order of appearance in the DOM tree, meaning it will first attempt to scroll the window, and narrow its focus down rather than the old behaviour of looking at scrollable ancestors in order of closeness to the draggable element in the DOM tree (reversed tree order).
This generally leads to an improved user experience, but can be customized by passing a configuration object to the
autoScroll
prop that sets theorder
option toTraversalOrder.ReversedTreeOrder
instead of the new default value ofTraversalOrder.TreeOrder
:The autoscrolling
thresholds
,acceleration
andinterval
can now also be customized using theautoScroll
prop:Finally, consumers can now conditionally opt out of scrolling certain scrollable ancestors using the
canScroll
option of theautoScroll
prop: