-
Notifications
You must be signed in to change notification settings - Fork 6.8k
feat(drag-drop): add support for automatic scrolling #16382
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
feat(drag-drop): add support for automatic scrolling #16382
Conversation
aa93d82
to
b4be85f
Compare
Just approve it! Can't wait! |
is there any live demo? |
this._dropContainer._stopScrolling(); | ||
this._animatePreviewToPlaceholder().then(() => { | ||
this._cleanupDragArtifacts(event); | ||
this._dragDropRegistry.stopDragging(this); |
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.
Would it make sense for stopDragging
to cause _cleanupDragArtifacts
in itself?
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.
_cleanupDragArtifacts
is private to the DragRef
whereas stopDragging
is in the DragDropRegistry
. Also this logic isn't new, I just moved it around so it's easier to follow.
const clientRect = {width, height, top: 0, right: width, bottom: height, left: 0}; | ||
verticalScrollDirection = getVerticalScrollDirection(clientRect, pointerY); | ||
horizontalScrollDirection = getHorizontalScrollDirection(clientRect, pointerX); | ||
scrollNode = window; |
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.
Was there some issue about window
not existing in some environment? I vaguely recall this but don't remember the details
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.
It doesn't exist during server-side rendering, but the only way the user can hit this code path is after a specifier sequence of mousedown
and mousemove
events hence why I haven't added extra guards around it.
* Adds support for automatically scrolling either the list or the viewport when the user's cursor gets within a certain threshold of the edges (currently within 5% inside and outside). * Handles changes to the scroll position of both the list and the viewport while the user is dragging. Previous our positioning would break down and we'd emit incorrect data. * No longer blocks the mouse wheel scrolling while the user is dragging. * Allows the consumer to opt out of the automatic scrolling. Fixes angular#13588.
b4be85f
to
156361a
Compare
Reworked based on the feedback @jelbourn. |
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.
LGTM
It looks nice! |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
Fixes #13588.

