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

Allow autoscroll for items larger than the scrolling parent #616

Open
robertontiu opened this issue Jul 22, 2023 · 2 comments
Open

Allow autoscroll for items larger than the scrolling parent #616

robertontiu opened this issue Jul 22, 2023 · 2 comments

Comments

@robertontiu
Copy link

Description

We ran into this issue where draggable children can be substantially larger than the scrolling parent and, to my surprise after digging into your source code, I noticed you intentionally disable auto scrolling in these situations. I started to understand the implications after spending a couple of hours reading through, but I had the feeling this can be implemented in a fairly simple way.

Proposed solution

  • Use mouse position relative to scrolling container when determining the auto scroll direction instead
  • Remove adjustForSizeLimits logic
  • Change distanceToEdges from edge -> center to edge -> edge (fx top -> top,)

Patch package diff

diff --git a/node_modules/@hello-pangea/dnd/dist/dnd.esm.js b/node_modules/@hello-pangea/dnd/dist/dnd.esm.js
index 82fe002..c20ebf4 100644
--- a/node_modules/@hello-pangea/dnd/dist/dnd.esm.js
+++ b/node_modules/@hello-pangea/dnd/dist/dnd.esm.js
@@ -8,6 +8,8 @@ import memoizeOne from 'memoize-one';
 import rafSchd from 'raf-schd';
 import _extends from '@babel/runtime/helpers/esm/extends';
 
+let globalMousePosition;
+
 const isProduction$1 = process.env.NODE_ENV === 'production';
 
 const spacesAndTabs = /[ \t]{2,}/g;
@@ -4054,13 +4056,14 @@ var getScrollOnAxis = (_ref => {
   let {
     container,
     distanceToEdges,
+    distanceToMouse,
     dragStartTime,
     axis,
     shouldUseTimeDampening,
     getAutoScrollerOptions
   } = _ref;
   const thresholds = getDistanceThresholds(container, axis, getAutoScrollerOptions);
-  const isCloserToEnd = distanceToEdges[axis.end] < distanceToEdges[axis.start];
+  const isCloserToEnd = distanceToMouse[axis.end] < distanceToMouse[axis.start];
   if (isCloserToEnd) {
     return getValue({
       distanceToEdge: distanceToEdges[axis.end],
@@ -4112,16 +4115,36 @@ var getScroll$1 = (_ref => {
     shouldUseTimeDampening,
     getAutoScrollerOptions
   } = _ref;
-  const distanceToEdges = {
+
+  if (!globalMousePosition) {
+    return null;
+  }
+
+  const distanceToEdgesFromCenter = {
     top: center.y - container.top,
     right: container.right - center.x,
     bottom: container.bottom - center.y,
     left: center.x - container.left
   };
 
+  const distanceToEdges = {
+    top: distanceToEdgesFromCenter.top - subject.height / 2,
+    right: distanceToEdgesFromCenter.right - subject.width / 2,
+    bottom: distanceToEdgesFromCenter.bottom - subject.height / 2,
+    left: distanceToEdgesFromCenter.left - subject.width / 2
+  }
+
+  const distanceToMouse = {
+    top: globalMousePosition.x - container.top,
+    right: container.right - globalMousePosition.x,
+    bottom: container.bottom - globalMousePosition.y,
+    left: globalMousePosition.x - container.left
+  }
+
   const y = getScrollOnAxis({
     container,
     distanceToEdges,
+    distanceToMouse,
     dragStartTime,
     axis: vertical,
     shouldUseTimeDampening,
@@ -4130,6 +4153,7 @@ var getScroll$1 = (_ref => {
   const x = getScrollOnAxis({
     container,
     distanceToEdges,
+    distanceToMouse,
     dragStartTime,
     axis: horizontal,
     shouldUseTimeDampening,
@@ -4144,15 +4168,7 @@ var getScroll$1 = (_ref => {
     return null;
   }
 
-  const limited = adjustForSizeLimits({
-    container,
-    subject,
-    proposedScroll: required
-  });
-  if (!limited) {
-    return null;
-  }
-  return isEqual$1(limited, origin) ? null : limited;
+  return required;
 });
 
 const smallestSigned = apply(value => {
@@ -5335,6 +5351,7 @@ function getCaptureBindings(_ref) {
       if (phase.type === 'DRAGGING') {
         event.preventDefault();
         phase.actions.move(point);
+        globalMousePosition = point;
         return;
       }
 
@@ -5356,6 +5373,7 @@ function getCaptureBindings(_ref) {
   }, {
     eventName: 'mouseup',
     fn: event => {
+      globalMousePosition = null;
       const phase = getPhase();
       if (phase.type !== 'DRAGGING') {
         cancel();
@@ -5371,6 +5389,10 @@ function getCaptureBindings(_ref) {
   }, {
     eventName: 'mousedown',
     fn: event => {
+      globalMousePosition = {
+        x: event.clientX,
+        y: event.clientY
+      };
       if (getPhase().type === 'DRAGGING') {
         event.preventDefault();
       }
@CodrinSocol
Copy link

Hey @robertontiu! I stumbled across the same issue as you. Would you mind creating a PR with your fix? Maybe one of the devs will approve the fix. Thanks!

@robertontiu
Copy link
Author

@CodrinSocol
I am reluctant to do this because a proper solution would most likely not use a global mousePosition variable and I'm not familiar enough with the code base (or have time at this point) to add that. I will leave it to the developers who actively maintain this library to do as they see fit. In the meantime, you can apply this patch with patch-package.

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

No branches or pull requests

2 participants