components icon indicating copy to clipboard operation
components copied to clipboard

Cancel drag after hovering a drop zone

Open TKul6 opened this issue 5 years ago • 31 comments

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 avatar Oct 30 '18 15:10 TKul6

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,

TKul6 avatar Oct 30 '18 15:10 TKul6

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.

crisbeto avatar Oct 30 '18 18:10 crisbeto

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 avatar Oct 30 '18 19:10 TKul6

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

TKul6 avatar Oct 30 '18 20:10 TKul6

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.

Alex6015 avatar Oct 31 '18 09:10 Alex6015

@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.

crisbeto avatar Oct 31 '18 16:10 crisbeto

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 avatar Oct 31 '18 19:10 TKul6

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

Thanks

TKul6 avatar Nov 02 '18 17:11 TKul6

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

ngbot[bot] avatar Nov 07 '18 18:11 ngbot[bot]

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

ngbot[bot] avatar Nov 07 '18 18:11 ngbot[bot]

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,

TKul6 avatar Dec 10 '18 08:12 TKul6

@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.

devversion avatar Dec 10 '18 08:12 devversion

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

TKul6 avatar Dec 10 '18 13:12 TKul6

@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.

devversion avatar Dec 10 '18 17:12 devversion

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

TKul6 avatar Dec 10 '18 20:12 TKul6

@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.

devversion avatar Dec 10 '18 21:12 devversion

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,

TKul6 avatar Dec 11 '18 08:12 TKul6

@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.

devversion avatar Dec 11 '18 11:12 devversion

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

TKul6 avatar Dec 12 '18 19:12 TKul6

Any news? I need this feature

Thanks!

Alex6015 avatar Dec 16 '18 16:12 Alex6015

@crisbeto @jelbourn any news?

TKul6 avatar Dec 16 '18 20:12 TKul6

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?

crisbeto avatar Dec 17 '18 21:12 crisbeto

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,

TKul6 avatar Dec 17 '18 21:12 TKul6

@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.

crisbeto avatar Dec 21 '18 19:12 crisbeto

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

Thanks a lot

TKul6 avatar Dec 21 '18 20:12 TKul6

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

crisbeto avatar Dec 21 '18 20:12 crisbeto

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,

TKul6 avatar Dec 22 '18 08:12 TKul6

@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.

crisbeto avatar Dec 23 '18 13:12 crisbeto

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

TKul6 avatar Dec 29 '18 20:12 TKul6

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

image

leosun1221 avatar Jan 20 '19 20:01 leosun1221