Skip to content

refactor(drag-drop): add sorted event #13887

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

Merged
merged 1 commit into from
Nov 10, 2018

Conversation

crisbeto
Copy link
Member

Adds a new cdkDropListSorted event that emits the item's previous index and current index as it's being sorted in a list.

Fixes #13863.

@crisbeto crisbeto added the target: patch This PR is targeted for the next patch release label Oct 30, 2018
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 30, 2018
/** Event emitted when the user swaps the position of two drag items. */
export interface CdkDragSort<T = any, I = T> {
/** Index from which the item was sorted previously. */
previousIndex: number;
Copy link
Member

Choose a reason for hiding this comment

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

Will this be the index since the last time a sort event or since the beginning of the drag?

Should be provide the previousContainer and currentContainer?

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's since the last sort. I don't know how useful the previousContainer will be in this case and it'll add some complexity since we don't have it easily available in _sortItem.

@josephperrott
Copy link
Member

This should be marked as feat rather than refactor correct?

@crisbeto
Copy link
Member Author

I wasn't sure exactly how to mark it, because on one hand it's a new public API, but on the other it's just exposing some info that we had already.

@@ -101,6 +101,10 @@ export class CdkDropList<T = any> implements OnInit, OnDestroy {
@Output('cdkDropListExited')
exited: EventEmitter<CdkDragExit<T>> = new EventEmitter<CdkDragExit<T>>();

/** Emits as the user is swapping items while dragging. */
Copy link
Member

Choose a reason for hiding this comment

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

Could you say "actively dragging" just to make it more clear that it while the interaction is occurring?

@@ -71,3 +71,15 @@ export interface CdkDragMove<T = any> {
*/
delta: {x: -1 | 0 | 1, y: -1 | 0 | 1};
}

/** Event emitted when the user swaps the position of two drag items. */
export interface CdkDragSort<T = any, I = T> {
Copy link
Member

Choose a reason for hiding this comment

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

Can you call this CdkDragSortEvent? I forgot to mention this, but for v8 we want to make all of our emitted event objects consistently named with Event at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but it'll be the odd one out since none of the other events have it.

@crisbeto crisbeto force-pushed the 13863/drag-sorted-event branch from 3f89c36 to b848344 Compare October 31, 2018 20:02
@crisbeto
Copy link
Member Author

The feedback has been addressed.

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 Nov 6, 2018
@ngbot
Copy link

ngbot bot commented Nov 6, 2018

I see that you just added the pr: merge ready label, but the following checks are still failing:
    failure conflicts with base branch "master"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@jelbourn
Copy link
Member

jelbourn commented Nov 6, 2018

Just needs a rebase now

@ngbot
Copy link

ngbot bot commented Nov 7, 2018

Hi @crisbeto! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@crisbeto crisbeto force-pushed the 13863/drag-sorted-event branch from b848344 to ce567b8 Compare November 8, 2018 08:24
Adds a new `cdkDropListSorted` event that emits the item's previous index and current index as it's being sorted in a list.

Fixes angular#13863.
@vivian-hu-zz vivian-hu-zz merged commit 26c73e0 into angular:master Nov 10, 2018
vivian-hu-zz pushed a commit that referenced this pull request Nov 12, 2018
Adds a new `cdkDropListSorted` event that emits the item's previous index and current index as it's being sorted in a list.

Fixes #13863.
@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 10, 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 target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Emit Drop Target Index Change
5 participants