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

Cancel drag after hovering a drop zone #13879

Closed
wants to merge 12 commits into from
Closed

Conversation

TKul6
Copy link

@TKul6 TKul6 commented Oct 30, 2018

I noticed that when a drag item being drag over a drop zone it is impossible to cancel the drag.
This stackblitz can demonstrate the problem:
https://stackblitz.com/angular/lmgeabvykokl
If you drag item from the To do list over the Done list and then drop it outside the done drop zone the item will land in the done zone.

This is not an expected behavior, if I dragged an item to a drop zone, but then drop it outside the drop zone, then the item should be returned to the initial container.

I think my fix is very straight forward: in the _updateActiveDropContainer method I check if the item is still over the drop container, if it doesn't I assign the _initalContainer to be the new drop container.

TKul6 added 2 commits October 30, 2018 11:58
Dragging an item over a drop zone and dropping it outside the zone cancels the drag.
Test a use case when the client drags an element over a drop zone and drop it outside the zone.
@TKul6 TKul6 requested a review from crisbeto as a code owner October 30, 2018 15:36
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 30, 2018
@TKul6
Copy link
Author

TKul6 commented Oct 30, 2018

Hi @crisbeto, can you help with the CI, I'm not sure I understand the problem from the error. The entire tests suite passed locally...
Thanks,

@crisbeto
Copy link
Member

crisbeto commented Oct 30, 2018

I don't think that this is the most common use case or the more intuitive behavior. When the user moved the item over to the new list, they already got some feedback that the item was transferred. Having it go back to a different one while also removing the placeholder from the current container looks confusing.

@TKul6
Copy link
Author

TKul6 commented Oct 30, 2018

Thanks for the comment @crisbeto,
I agree this is not the most common use case, but I think it is still the a valid use case.
Maybe I didn't explain myself clearly, the use case I refer is:

A user start a drag process, drag the item over a valid drop zone, and decides to cancel it by dropping the dragged item in an invalid drop zone

I'm sorry but I do believe this scenario may happen (it happened to me several times).

Now, according to the current implementation the drag process will not be canceled, but completed (even though the user dropped the item in an invalid zone), the user would have to take the item back to the initial drop zone by himself.

Think of a use case that you have more than source drop zone and target drop zone, now the user must remember the source drop zone to drop the item back.

  • Try to drag an email in gmail from the Inbox over to a target folder, but then drop it in the background. Does the mail moved to the target folder?
  • In your computer, Try to drag a file over a folder and then drop it somewhere else Does the file moved inside the folder?

This is the use case I'm trying to solve, and I tried to implement a solution based on my experience with drag and drop cases as I mentioned earlier.
Maybe the way I solved it is not the perfect way (and if you have any ideas how to improve that please share).
However, I do believe this use case deserves attention.

What do you say?

@TKul6
Copy link
Author

TKul6 commented Oct 30, 2018

There might be a potential problem, currently when the item dragged out from a container the cdkDropListExited emitter emits a value. Perhaps in the case the item was dragged over the container a different emitter should be invoked (cdkDropListLeft) or do not emit a value at all.

What do you think?
Thanks

@Alex6015
Copy link

I don't think that this is the most common use case or the more intuitive behavior. When the user moved the item over to the new list, they already got some feedback that the item was transferred. Having it go back to a different one while also removing the placeholder from the current container looks confusing.

Hi,
I'm having the same issue with drag and drop, i think @TKul6 is right, if i move the mouse pointer to the edge of the screen i expect the drop to be cancelled, but that's not the case.

@crisbeto
Copy link
Member

@TKul6 I think that we should be able to support your case, but it shouldn't be the default behavior. I'm open to ideas about what the API could look like.

@TKul6
Copy link
Author

TKul6 commented Oct 31, 2018

OK, I agree it may change behavior in existing applications.
What about adding an additional property to the CdkDragConfig called InvalidDropStrategy (or something like that)?

If I understand the an Angular InjectionToken correctly, it gives me the ability to override the factory in my component (or module).

That way, when a drag item is created, it will have an LastContainer DropStrategy (the default behavior) and in my module / component I'll override the config with InitialContainer DropStrategy.

So:

  • Default behavior is not changed.
  • The user can decide what happens when item is dropped in an invalid drop zone.
  • The change is made in one place and injected to any drag item.

What do you say @crisbeto?
Thanks a lot

@TKul6
Copy link
Author

TKul6 commented Nov 2, 2018

Hi @crisbeto have you had a chance to consider my suggested solution?

Thanks

Adding to CDK_DRAG_CONFIG additional parameter to support cancel dropping in invalid drop zone.
@ngbot
Copy link

ngbot bot commented Nov 7, 2018

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

1 similar comment
@ngbot
Copy link

ngbot bot commented Nov 7, 2018

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

@TKul6
Copy link
Author

TKul6 commented Dec 10, 2018

Hi @jelbourn can you please help me with the Bazel issue, I'm not sure what's the problem or how to handle it.

Thanks,

@devversion
Copy link
Member

devversion commented Dec 10, 2018

@TKul6 You need to have bazel installed and should run bazel run //tools/public_api_guard:cdk_drag-drop_api.accept.

@jelbourn Since we didn't declare bazel as a development requirement (yet), should we go down the road of providing Bazel through yarn? Similar to how I did it for angular/angular? I'd prefer not running Bazel through Yarn personally but it makes it easier for people without Bazel installed.

@TKul6
Copy link
Author

TKul6 commented Dec 10, 2018

Thanks a lot @devversion, I have now Bazel working however I get this error while running: Analysis of target '//tools/public_api_guard:cdk_drag-drop_api.accept' failed; build aborted: no such package '@matdeps//typescript': I'm not sure if it's something I messed up, I do believe the typescript was required before my changes.

Do you have any suggestion how to investigate this issue?

Thanks

@devversion
Copy link
Member

@TKul6 That looks new to me. Can you send the whole log output? Might be worth clearing the cache using bazel clean --expunge so see if it installs the matdeps properly.

I will work on a markdown document for Bazel soon. This should make it more clear.

@TKul6
Copy link
Author

TKul6 commented Dec 10, 2018

Thanks @devversion ,
I tried to clean but it didn't help.

Here's the complete log:

bazel run //tools/public_api_guard:cdk_drag-drop_api.accept
WARNING: The following rc files are no longer being read, please transfer their contents or import their path into one of the standard rc files:
c:\github\material2/.bazelrc
Starting local Bazel server and connecting to it...
INFO: Invocation ID: c0e60d81-c689-4ab2-a391-24b6fef4f5de
Loading:
Loading: 0 packages loaded
Loading: 0 packages loaded
INFO: SHA256 (https://github.com/bazelbuild/rules_typescript/archive/0.21.0.zip) = e4f51c408ed3278a3a1dd227564a69f293ae2ac4ae1564b3a6d2637ae9447b47
Loading: 0 packages loaded
INFO: SHA256 (https://github.com/bazelbuild/rules_nodejs/archive/0.16.3.zip) = b996a3ce55c49ae359468dae040b30025fdc0917f67b08c36929ecb1ea02907e
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
INFO: SHA256 (https://github.com/angular/angular/archive/7.1.2.zip) = f7ac4e29adf0b9f45db9fb266736443b399191f37718e2a692bfc17070da8589
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
INFO: SHA256 (https://github.com/bazelbuild/rules_sass/archive/1.15.2.zip) = 96cedd370d8b87759c8b4a94e6e1c3bef7c17762770215c65864d9fba40f07cf
Loading: 0 packages loaded
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (3 packages loaded, 0 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (10 packages loaded, 40 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (10 packages loaded, 40 targets configured)
yarn install v1.12.1
[1/4] Resolving packages...
[2/4] Fetching packages...
[3/4] Linking dependencies...
[4/4] Building fresh packages...
Done in 5.55s.
yarn install v1.12.1
$ node ./tools/npm/check-npm.js
[1/5] Validating package.json...
[2/5] Resolving packages...
[3/5] Fetching packages...
info [email protected]: The platform "win32" is incompatible with this module.
info "[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
info @google-cloud/[email protected]: The engine "node" is incompatible with this module. Expected version "~6". Got "10.10.0"
info "@google-cloud/[email protected]" is an optional dependency and failed compatibility check. Excluding it from installation.
[4/5] Linking dependencies...
warning "@angular/bazel > [email protected]" has incorrect peer dependency "typescript@>=2.4.2 <2.10".
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (71 packages loaded, 3819 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (71 packages loaded, 3819 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (71 packages loaded, 3819 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (71 packages loaded, 3819 targets configured)
Analyzing: target //tools/public_api_guard:cdk_drag-drop_api.accept (71 packages loaded, 3819 targets configured)
ERROR: Analysis of target '//tools/public_api_guard:cdk_drag-drop_api.accept' failed; build aborted: no such package '@matdeps//typescript': yarn_install failed: (Timed out)
INFO: Elapsed time: 897.777s
INFO: 0 processes.
FAILED: Build did NOT complete successfully (71 packages loaded, 3819 targets configured)
ERROR: Build failed. Not running target
FAILED: Build did NOT complete successfully (71 packages loaded, 3819 targets configured)

Any suggestion how to proceed?

Thanks

@devversion
Copy link
Member

@TKul6 Thanks! Looks like yarn install for some reason timed out. It shouldn't take longer than 15min (which it took for you right)? Can you please double-check your internet connection or PC utilization.

Otherwise you should be able to just manually make changes as the diff shows in bazel_build_test. I will try to debug your issue, but for some reason Yarn doesn't run "fast enough" for you.

@TKul6
Copy link
Author

TKul6 commented Dec 11, 2018

Thanks @devversion, it seems closing some applications did the trick.

However, now the e2e tests are failing, from the logs (in CircleCI) it seems the chrome version is wrong?:
SessionNotCreatedError: session not created: Chrome version must be between 70 and 73

Is it related to ciecleCI, I saw the Selenium server is up and running, Is there anywhere in the project to control the chrome version???
Thanks,

@devversion
Copy link
Member

@TKul6 Please don't worry about it. I've created a PR to fix this #14461. @crisbeto this should be ready for review (again) I think.

@TKul6
Copy link
Author

TKul6 commented Dec 12, 2018

Thanks @devversion.
@crisbeto can you please take a look (I don't want to handle the merges again...)

The most critical changes are here and here the other stuff is just handling CI stuff.

Thank you all

@Alex6015
Copy link

Any news?
I need this feature

Thanks!

@TKul6
Copy link
Author

TKul6 commented Dec 16, 2018

@crisbeto @jelbourn any news?

@crisbeto
Copy link
Member

Sorry for the delayed response @TKul6. I think the problem with this approach is that we'd have to have the CDK know about all the possible strategies and have it react accordingly with the various combinations of it and the inputs. Wouldn't it be more flexible if we made the _updateActiveDropContainer protected which means that you can extend the class and provide your own logic?

@TKul6
Copy link
Author

TKul6 commented Dec 17, 2018

Hi @crisbeto ,
I understand what you're saying.
Personally I don't have a problem with the extend approach, except the fear from the future, where there might be multiple descenders of the original drag item where every class changes a small specific thing.

Maybe, since we change the behavior, but we still need 99% of the original drag class, we should extract the container update logic to external service that will be an optional injected service to the drag class. this way we keep one clean class and support for any kind of dropping strategy that might be needed in the future (just extend the service and override the method).

And in this way the cdk doesn't need to know anything about strategy. It doesn't need to know even how to update the target container. All he knows there is some service he can query or invoke in order to get the updated container.

What do you think? shell we continue with your approach or mine?
Thanks,

@crisbeto
Copy link
Member

@TKul6 that does sound interesting. I was planning on having another service or moving some of the logic to the DragDropRegistry since the current approach where the logic is inside the DropListRef won't allow us to add more types of drop containers easily.

@TKul6
Copy link
Author

TKul6 commented Dec 21, 2018

Great, thanks @crisbeto .
I'll start working on the external service (I pretty sure it will be done by tomorrow).

Thanks a lot

@crisbeto
Copy link
Member

We appreciate the enthusiasm, but something like this would have to fit into the existing planned work for drag and drop.

@TKul6
Copy link
Author

TKul6 commented Dec 22, 2018

Sorry @crisbeto but I think I lost you.
Are you saying is something that you should do as part of a future development?
Is there something I can do in order to speed the process?
Thanks,

@crisbeto
Copy link
Member

@TKul6 I'm afraid that you would end up wasting your time by doing it now since we'll have to move things around it in the future, in order to support more use cases that we have planned.

@TKul6
Copy link
Author

TKul6 commented Dec 29, 2018

Hi @crisbeto,
Do you any estimation when can we resume the work on this feature (I really need it).

@leosun1221
Copy link

@TKul6 I have similar issue , I use isPointerOverContainer in cdkDropListDropped output to check if it is inside the drop area.

image

@mmalerba mmalerba added aaa and removed aaa labels Apr 25, 2019
@ShakinFire
Copy link

ShakinFire commented Oct 18, 2019

Hey guys, is there any resolution about this? There is still no proper way of cancelling the dropping of an item.

@devversion devversion removed their request for review August 18, 2021 12:55
@andrewseguin andrewseguin removed the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 29, 2021
@josephperrott josephperrott added action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews and removed needs rebase labels Nov 16, 2022
@josephperrott josephperrott requested a review from a team as a code owner December 18, 2024 17:40
@andrewseguin andrewseguin self-assigned this Jan 8, 2025
@andrewseguin
Copy link
Contributor

Unfortunately this PR is unlikely to land since too much had changed in the drag implementation for this code to be applicable. I opened #30333 to track this feature request. If anyone wants to open a new updated PR with this feature, we'd be happy to take contributions and give feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants