Skip to content

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jun 25, 2019

  • 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. Previously 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 #13588.
list-demo
viewport-demo

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 25, 2019
@crisbeto crisbeto force-pushed the 13588/drag-drop-viewport-scroll branch 2 times, most recently from aa93d82 to b4be85f Compare June 25, 2019 20:47
@crisbeto crisbeto added P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release labels Jun 25, 2019
@crisbeto crisbeto marked this pull request as ready for review June 25, 2019 21:12
@Roman-Simik
Copy link

Just approve it! Can't wait!

@Oneia
Copy link

Oneia commented Jul 1, 2019

is there any live demo?
can I scroll content without a dragging if table takes whole width?

this._dropContainer._stopScrolling();
this._animatePreviewToPlaceholder().then(() => {
this._cleanupDragArtifacts(event);
this._dragDropRegistry.stopDragging(this);
Copy link
Member

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?

Copy link
Member Author

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;
Copy link
Member

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

Copy link
Member Author

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.
@crisbeto crisbeto force-pushed the 13588/drag-drop-viewport-scroll branch from b4be85f to 156361a Compare July 2, 2019 04:21
@crisbeto
Copy link
Member Author

crisbeto commented Jul 2, 2019

Reworked based on the feedback @jelbourn.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jul 2, 2019
@jelbourn jelbourn merged commit 207dba6 into angular:master Jul 9, 2019
@SandorSzilard
Copy link

It looks nice!
Can we get a demo how to use it? (Or is it needed?)

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

drag-drop: make the view scroll when trying to move a draggable outside the current view
6 participants