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

fix: improve drag behaviour when using handle #2407

Merged
merged 1 commit into from Nov 14, 2022
Merged

Conversation

nunogois
Copy link
Member

@nunogois nunogois commented Nov 11, 2022

Fixes a small bug that caused the item to be draggable when clicking outside of the handle, when using a handle. Basically resets the draggable state when (re)entering the container.

@vercel
Copy link

vercel bot commented Nov 11, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
unleash-monorepo-frontend ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Nov 11, 2022 at 2:26PM (UTC)
1 Ignored Deployment
Name Status Preview Comments Updated
unleash-docs ⬜️ Ignored (Inspect) Nov 11, 2022 at 2:26PM (UTC)

@@ -74,6 +74,9 @@ const addEventListeners = (

handleEl.addEventListener('mouseenter', onMouseEnter);
handleEl.addEventListener('mouseleave', onMouseLeave);
if (handle) {
el.addEventListener('mouseenter', onMouseLeave);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: should line 75 be on the else statement?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I don't think it should, but I'm curious about your reasoning. We want onMouseEnter on handleEl every time - Whenever the mouse enters the element that will act as our handle, whether that's a specific "handle" or our whole element, we want it to enable the draggable state:

const onMouseEnter = (e: MouseEvent) => {
if (e.target === handleEl) {
el.draggable = true;
}
};

What this PR adds, is that whenever we're using the handle parameter (which means drag only on a specific "handle", not the whole element), we want to reset the draggable state when we enter (or re-enter) the container el.
image

So:

  • Entering our handleEl should enable the draggable state (line 75) - This applies whether we're using a specific "handle" or not;
  • Entering (or re-entering) the container el, our whole element, should reset the state (line 78), but only when using a specific "handle", otherwise it breaks the default behaviour of the whole thing being draggable;

Maybe it's easier to understand if you checkout the branch and try it yourself. To toggle between "drag only on handle" and "everything draggable" you can comment this line:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh! Thanks for the clarification! I didn't realize they were targeting different elements. The names didn't help, but I think it's not easy to give proper names to elements in the UI, it becomes too specific.

@nunogois nunogois merged commit bd686eb into main Nov 14, 2022
@nunogois nunogois deleted the fix-drag-with-handle branch November 14, 2022 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants