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

Allow dynamic drop action selection #107

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ViRuSTriNiTy
Copy link
Collaborator

@ViRuSTriNiTy ViRuSTriNiTy commented Mar 9, 2021

EventCallback approach discussed in relates to #106

This approach tries not to break existing implementations by defaulting to a move action. However consumers that currently use CopyItem parameter need to change the implementation as the callback that executes the actual copy is now assigned to CopyDropAction.

This approach uses child components to model drop actions.

…allow dynamic selection of drop action based on given arguments (default drop action is now a no-op)
…efault drop action to provide a sensible default (component can be used without much coding)
@ViRuSTriNiTy ViRuSTriNiTy changed the title Added event OnSelectDropAction to allow dynamic drop action selection Allow dynamic drop action selection Mar 9, 2021
@ViRuSTriNiTy ViRuSTriNiTy force-pushed the issue-106-drop-action-selection-in-event-handler branch from 2ae6d77 to 1febece Compare March 9, 2021 14:30
@Postlagerkarte
Copy link
Owner

Postlagerkarte commented Mar 9, 2021

I understand the approach and it looks flexible. You can combine actions and even create your own actions.
However, the actions are still opinionated, aren't they? e.g. how to tell the copy action where exactly to insert a new item?
Also available drop actions are hard to discover because there is no intellisense for them.
Any reason you don't want to simply expose the drop event including context info and let the user do all the list manipulation on his own?

@ViRuSTriNiTy
Copy link
Collaborator Author

ViRuSTriNiTy commented Mar 10, 2021

@Postlagerkarte

However, the actions are still opinionated, aren't they?

Yep, that's right. In this PR i'm primarily focusing on the initial issue title "Dropzone should not modify source and destination item list by default" which is the case when merging the PR. The provided actions in this package are basically a preset for consumers.

How to tell the copy action where exactly to insert a new item?

This info is currently passed via the TargetItemIndex property of the context class. I did not change anything here, that is a info that was passed around before this PR and it makes sense to me. Additional info can always be added to the context if it makes sense.

Also available drop actions are hard to discover because there is no intellisense for them.

Imho this is primarily an issue of the Razor editor and it affects all components not just this package. One of the workarounds would be to start searching from the interface definition and then search for implementations. Another option would be to generate a documentation for the user from the classes etc.

Any reason you don't want to simply expose the drop event including context info and let the user do all the list manipulation on his own?

The reason is to provide a easy get started experience. I always try to design components from the view of a consumer and that consumer might not want to add multiple lines of code to get started. Anyway, if you want to manipulate lists on your own then you don't register any drop action and handle OnItemDrop. Does this make sense?

@Postlagerkarte
Copy link
Owner

Yep, makes sense. I like it.

Currently all actions have the suffix DropAction. Might increase readability if we drop it.

        <DropActions>
            <MoveDropAction TItem="TodoItem" />
        </DropActions>
        <DropActions>
            <MoveItem TItem="TodoItem" />
        </DropActions>

What's your take?

@ViRuSTriNiTy
Copy link
Collaborator Author

I named the actions with the suffix DropAction because ... yeah, I basically try to mimic other components in my project and more importantly I always try to follow established naming conventions of .NET classes. The naming convention normally goes like this: interface IFooBar > class MyFooBar or YourFooBar etc.

I like your suggestion at the first glance as it provides shorter names but it would violate established naming conventions and I think it is difficult to find a suitable name for the interface and base class now. What's you idea here?

@Postlagerkarte
Copy link
Owner

Don't worry about that - since you can implement several interfaces it does not make sense to have an interface decide about the naming of classes. I will take a close look at the weekend.

@ViRuSTriNiTy
Copy link
Collaborator Author

@Postlagerkarte Any news from your side? 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants