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.

Sign up for GitHub

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

Jump to bottom

223 - added a cancelable USER_DROPPED consider event on the origin zone #232

Draft
wants to merge 9 commits into
base: master
Choose a base branch
from
Draft

223 - added a cancelable USER_DROPPED consider event on the origin zone #232

wants to merge 9 commits into from

Conversation

isaacHagoel
Copy link
Owner

@isaacHagoel isaacHagoel commented Jan 19, 2021 •
edited
Loading

added a cancelable USER_DROPPED consider event on the origin zone if the dragged element was dropped outside of any. cancelling skips the final animation to place and dispatches the finalize events right away.

the info provided in the event:

{
            trigger: TRIGGERS.USER_DROPPED,
            id: draggedElData[ITEM_ID_KEY],
            source: SOURCES.POINTER,
            draggedElement: draggedEl,
            pointerClientXY: {...currentMousePosition}
        }

example usage within the consider handler:

  if (e.detail.info.trigger === TRIGGERS.USER_DROPPED) {
            console.log("DROPPED", e.detail.info);
            e.preventDefault();
  }

missing updates to README and types

resolves #223

@isaacHagoel
223 - added a cancelable USER_DROPPED consider event on the origin zo…
89a57c8 
…ne if the dragged element was dropped outside of any. cancelling skips the final animation to place and dispatches the finalize events right away
@isaacHagoel
223 - added relevant info to the event
83a2972
@isaacHagoel isaacHagoel mentioned this pull request Jan 19, 2021
Closed
@canadaduane
Copy link

This looks good. I will try to use in my dnd implementation in Relm and see if anything comes up.

@isaacHagoel
Copy link
Owner Author

let me know if you have any feedback. I am hoping to spend more time on this over the weekend

@canadaduane
Copy link

canadaduane commented Jan 21, 2021 via email

@canadaduane
Copy link

All my tests check out! This accomplishes what my 'customDrop' PR does and without introducing a new config option.

canadaduane added a commit to canadaduane/svelte-dnd-action-custom-drop that referenced this pull request Jan 22, 2021
@canadaduane
Update to use TRIGGERS.USER_DROPPED
678f0b2 
See isaacHagoel/svelte-dnd-action#232
@isaacHagoel
Copy link
Owner Author

mmm... i've been playing with it but on my new example (with files and folders) it looks like for some reason the event is always cancelled (without me calling prevent default). I am not sure why. still investigating.

@isaacHagoel
Copy link
Owner Author

alright. i fixed it. now it behaves correctly in both cases. will proceed testing it in context later.

@Florian-Schoenherr
Copy link

Sorry, I was blocked this week, can take a look later. If this helps his usecase, you can merge this at any time.

@canadaduane
Copy link

Is this ready to be merged in?

@isaacHagoel
Copy link
Owner Author

isaacHagoel commented Feb 9, 2021 via email

@canadaduane
Copy link

Yes, it seems to work well. It was a near drop-in replacement for the customDrop approach I'd tried earlier (no pun intended!)

@isaacHagoel
Copy link
Owner Author

isaacHagoel commented Feb 9, 2021 via email

@canadaduane
Copy link

Thank you!

@isaacHagoel
Copy link
Owner Author

@canadaduane can you remind me pls... is there a reason i decided to dispatch the new "USER_DROPPED" event only when the dragged element is outside of any zone? why didn't i make the library dispatch it and allow cancelling the final animation regardless of where it was dropped (which makes more sense to me when i think about it now)?

also, note to self - instead of conditional logic around whether or not to play the animation we can set the duration of the animation to zero if the event was cancelled

@isaacHagoel
223 - merged master
d86b913
@isaacHagoel
223 - simplified the implementation of the user_dropped event. it now…
c752201 
… triggers on any pointer drop and change the duration of the animation to zero if cancelled instead of branching the code. added isDroppedOutsideOfAnyDzOfType as a property of the event.
@isaacHagoel
Copy link
Owner Author

reply to self: I did it this way because otherwise the user code doesn't have enough information to decide whether to cancel the drop or not (dropped where).
I streamlined the implementation and now it doesn't seem to cause any undesired side effects.
using it with the modified code:

const {items: newItems, info: {trigger, pointerClientXY, isDroppedOutsideOfAnyDzOfType}} = e.detail;
        if (trigger === TRIGGERS.USER_DROPPED && isDroppedOutsideOfAnyDzOfType) {
            e.preventDefault();
        }

Now I wonder whether this is the time and place to introduce a new event type in addition to consider and finalize. Maybe it should be called fyi or something like that. Otherwise this will cause everyone to needlessly re-render when they get this consider. not sure if it is a big deal.
If i do add the event, the DRAGGED _STOPPED keyboard event probably falls into this category too (breaking change - version will become 1.0.0).
Any thoughts are welcome.

Also, @Florian-Schoenherr did u get to try it with the whiteboard example?

@Florian-Schoenherr
Copy link

Florian-Schoenherr commented Feb 13, 2021 •
edited
Loading

I'm gonna try it now.
Edit: How would I go about doing that? 😄
I've got the branch on my end, but uhm.. how can I try it out?

@isaacHagoel isaacHagoel mentioned this pull request Mar 16, 2021
Closed
@isaacHagoel
Copy link
Owner Author

@Florian-Schoenherr sorry, i only saw your edit now. you use 'npm link' in order to make your repo use the local version of the library.
cd to the svelte-dnd-action folder and run npm link
cd to your project folder and run npm link svelte-dnd-action

@isaacHagoel isaacHagoel mentioned this pull request Apr 13, 2021
Open
@Florian-Schoenherr
Copy link

Florian-Schoenherr commented Apr 15, 2021 •
edited
Loading

Thank you, neat and helpful trick :)
image
The white background is a dnd-zone, holding the green cards, which are absolute positioned (with TRIGGERS.USER_DROPPED).
The pinkish rectangles inside the cards work like before (you can sort them and you can drag them outside to put them into their own card).

image
Inside handleDrop(), I added this for debugging, mainly because I get an error when I drop the last element of the card-list, shortly after this line.

image
Here, shadowElIdx: 2 is the index of the last element in the card-list, so getBoundingRectNoTransforms(shadowElDropZone.children[shadowElIdx]); will not work, because the child with index 2 is currently getting dragged.

@isaacHagoel
Copy link
Owner Author

not sure i follow. it might be an issue that was recently fixed on the master branch. can u pls provide some files with code that i can used in order to reproduce this error?

@isaacHagoel
Copy link
Owner Author

@Florian-Schoenherr ^

@Florian-Schoenherr
Copy link

Florian-Schoenherr commented Apr 17, 2021 •
edited
Loading

getBoundingRectNoTransforms(shadowElDropZone.children[shadowElIdx]);
in this case will do
getBoundingRectNoTransforms(shadowElDropZone.children[2]);
but
shadowElDropZone.children is [0: div, 1: div], so does not have an element at index 2.

But I found out what the bug was:
In one of the consider functions I did itemGroups = itemGroups instead of itemGroups = e.detail.items; 😅

Now after that change, I can do:

const considerGroup = (e) => {
	if (e.detail.info.trigger === TRIGGERS.USER_DROPPED) {
		const group = itemGroups.find(g => g.id === "id:dnd-shadow-placeholder-0000");
		group.x = e.detail.info.draggedElement.getBoundingClientRect().x;
		group.y = e.detail.info.draggedElement.getBoundingClientRect().y;
           	e.preventDefault();
	}
	itemGroups = e.detail.items;
}

but finalize still runs, so uhh, would I then replace the shadow item myself?
If I leave away finalize completely and instead of the above, change it to:

group.x = e.detail.info.draggedElement.getBoundingClientRect().x;
group.y = e.detail.info.draggedElement.getBoundingClientRect().y;
group.id = e.detail.info.id;
delete group["isDndShadowItem"];
e.preventDefault();

it works.

SHADOW_PLACEHOLDER_ITEM_ID doesn't seem to be exported any longer

Code (just focused on the group here)

@isaacHagoel
Copy link
Owner Author

isaacHagoel commented Apr 17, 2021 via email

@Florian-Schoenherr
Copy link

@isaacHagoel do you think the code in the REPL makes sense? I just hacked that together somehow, seemed a little like I was fighting the intended way, otherwise yeah, it works.

@isaacHagoel
Copy link
Owner Author

isaacHagoel commented Apr 17, 2021 via email

@Florian-Schoenherr
Copy link

@isaacHagoel have a nice weekend :)

@isaacHagoel
Copy link
Owner Author

what is the desired behaviour in that REPL? freely dragging the groups around and be able to move items between groups?

@Florian-Schoenherr
Copy link

Yeah, dragging the groups around, moving between groups and when dragging one item outside a group, adding it to it's own, new group. It works but seems hacky.

@isaacHagoel
Copy link
Owner Author

isaacHagoel commented Apr 19, 2021 via email

@isaacHagoel
Copy link
Owner Author

I played with it a bit and indeed this usecase (especially the need to create a new group when the item is dropped outside of any, remove the item manually from the original group and so on) doesn't feel nice to implement.
I pushed a few updates to the branch:

  1. merged master
  2. added draggedItemData to the new event's info. This allows you to place it where it needs to go (ex: within a newly allocated group) without worrying about shadow item shenanigans (it is the original item's data).
    As i said, tbh i am still not sure on whether the library should support this kind of usecase or not. If the answer is yes it shoud, i think it should do it in a nicer way.

@Florian-Schoenherr
Copy link

@isaacHagoel will take a look soon.
I wonder if there's a way to keep all this dnd/whiteboard/Shopify-draggable usecase stuff idiomatic, easy to compose etc.
Of course you can't expand scope endlessly (although I think you've done a great job with the current functionality and would probably be able to do more of this kind of thing).

@canadaduane
Copy link

canadaduane commented Apr 20, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Event for getting current position
3 participants
@isaacHagoel @canadaduane @Florian-Schoenherr