blazor-dragdrop icon indicating copy to clipboard operation
blazor-dragdrop copied to clipboard

Dropzone should not modify source and destination item list by default

Open ViRuSTriNiTy opened this issue 3 years ago • 6 comments

Hi there,

the current implementation of the drop handling is strongly opinionated.

This makes is really difficult if not impossible to use the package in scenarios where you don't want to modify the source and destination item list by default. The reason for this would be the requirement to execute some logic after drop and only if the logic succeeds the operation is considered a success. Modifying source and destination right away makes no sense here as the logic can fail and modifications cannot easily be undone right now.

To avoid breaking existing implementations I suggest we add another event OnItemDropping. This event receives a parameter SkipDefaultHandling which can be set to true to skip the copy / swap / instant replace handling. The component can now skip the default handling, raise OnItemDrop, the component consumer can execute logic in this event and when the logic succeeds its up to the consumer to modify the lists accordingly.

@Postlagerkarte What do you say?

ViRuSTriNiTy avatar Mar 08 '21 14:03 ViRuSTriNiTy

Great! I would also be open to make the new custom handling the default and force users to set a flag to enable the old "auto" handling. Would make sense if we want to drop support for the old handling in a future version. Do you prefer that or should we keep the current as default and make the new way opt-in as suggested above?

Postlagerkarte avatar Mar 08 '21 18:03 Postlagerkarte

I was primarily searching for a way to extend the component but I'm also keen on adding a parameter that sets the default behavior to avoid increasing the complexity even further.

My suggestion is to add a parameter named DropAction of type enum whereas the enum describes the current actions possible plus an no-op action and / or callback action. This would also improve the readability of the code as currently the if else blocks are hard to grasp.

I guess I should create a PR and discuss things there when needed, right?

ViRuSTriNiTy avatar Mar 08 '21 22:03 ViRuSTriNiTy

Yes, go ahead!

Postlagerkarte avatar Mar 09 '21 08:03 Postlagerkarte

Is there a reason why OnDropItemOnSpacing and OnDrop is a synchronous method?

I ask this because I pretty much discarded the enum idea already as it just refactores the existing if else blocks into new if else blocks. Currently I'm trying the initial idea with a EventCallback but with a synchronous method I cannot await the EventCallback.

Also could you please leave the issue open until it is resolved / implemented? I'm tracking open issues instead of open PRs.

ViRuSTriNiTy avatar Mar 09 '21 09:03 ViRuSTriNiTy

No special reason for those method being synchronous. What about dropping the current implementation completely? I am not opposed to breaking changes if it is necessary and reduces complexity. As you pointed out the idea that the component handles the list manipulation is sub optimal anyways. I assume that migrating should be very easy for users. Don't you think so?

Postlagerkarte avatar Mar 09 '21 09:03 Postlagerkarte

What about dropping the current implementation completely? I am not opposed to breaking changes if it is necessary and reduces complexity. As you pointed out the idea that the component handles the list manipulation is sub optimal anyways.

Yes, I understand your point but dropping the current default behavior (move item from a to b) would break all existing implementations. I think new consumers of this component will expect the component to at least implement a drag & drop action by default to get started right away.

I have started a PR regarding the EventCallback approach. To me its a good solution as it enables custom actions, it defaults to move action to avoid breaking implementations as much as possible and code change is also minimal and I think the implementation of the action classes are as transparent as it gets.

Lets discuss the PR in review comments.

ViRuSTriNiTy avatar Mar 09 '21 10:03 ViRuSTriNiTy