mauricius / vue-draggable-resizable

Vue3 Component for draggable and resizable elements.
https://mauricius.github.io/vue-draggable-resizable/
MIT License
3.32k stars 558 forks source link

Draggable elements can be dragged outside the body #229

Open dvago opened 4 years ago

dvago commented 4 years ago

Hi there,

I was looking at the codebase to cover the enhancement request regarding the possibility to set the parent as CSS selector (see open issues) and half way through the implementation I had an idea which permits to solve the following issue:

Currently the resizable/draggable element could be dragged off the document, meaning the user could eventually lose control or visibility of elements unless the developer set parent to true:

out-of-body

I've a non-disruptive code change which I could push to achieve this:

constrained-to-body Note: within Storybook the white stage is the body

Would you mind think about it and let me know if you'd like to have this feature please? I could push this merge request into your package.

angeliquejw commented 4 years ago

@dvago, would you be willing to share a PR or gist that summarizes these changes? If they don't make it into this repo, we'd like to incorporate them into our local copy of this component. :)

dvago commented 4 years ago

I've ended up rebuilding the dialog system without this 3rd party package as the owner of the package did want to solve the issue 223 rather than quickly fix the above.

However, I will try to recover the change above and send it to you.

dvago commented 4 years ago

Here you go, https://gist.github.com/dvago/02aeab97cb0094ba1fc2219963333066

Basically, the getParentSize function returns either the width and the height of the parent If the parent flag is set to true, or the body width and height rather than null values.

Said so, the script has a few areas where it check for the parent flag in order to calculate boundary limits but because of the changes above there is no need of these checks as you want to have boundary limits anyway.

I wouldn't personally rely on this package to be honest with you, but I guess as workable script it's fine.

The main reason of the above sentence is because there is a system of $emitters on native listeners without any debounce mechanism making the whole package quite expensive, especially if you end up having multiple draggable/resizable elements.

VincentVanclef commented 4 years ago

@dvago Rather curious as to what you events you are talking about, could you ellaborate on what events that are making this package so expensive?

dvago commented 4 years ago

@VincentVanclef this: emitters

VincentVanclef commented 4 years ago

@dvago Well normal html5 drag events trigger just as much as this, and a debounce will give you incorrect drag results as you wont get the actual drag position until you stop dragging. Throttling might be a suitable solution tho, or saving the prev x/y positions as non reactive properties, and checking if new left/top !== prev left/top. That reduced it a hell lot:

https://i.imgur.com/yBXe1Ai.gif

dvago commented 4 years ago

Indeed, so you answered yourself ;) ... Also keep in mind that emitters are an extra layer on top of existing native listeners and you don't really need to bloat your script with them. (In fact, emitters are also bad practice in the new Vue version as far as I know)

Furthermore, emitting "dragging" could be easily avoided with some state e.g. "isDragging" => true/false which permits the user to use the state just if really needed or most likely just rely on hooks like "onDragStart"/"onDragEnd".

VincentVanclef commented 4 years ago

Ah that makes a lot of sense, thanks @dvago! Was unsure if i was on the right track :P

dvago commented 4 years ago

no problem, if you are building something you might want to maintain up to date in the mid term (e.g. next year) I would encourage you to read this article: https://dev.to/chenxeed/awesome-breaking-changes-in-vue-3-if-you-migrate-from-vue-2-3b98

VincentVanclef commented 4 years ago

Will do, ill give that article a read as well!