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

Only dispatch dragStart event if mouse is actually dragging #22

Closed
Tscheffrey opened this issue Sep 18, 2019 · 10 comments
Closed

Only dispatch dragStart event if mouse is actually dragging #22

Tscheffrey opened this issue Sep 18, 2019 · 10 comments
Labels
feature request New feature or request good first issue Good for newcomers

Comments

@Tscheffrey
Copy link

Tscheffrey commented Sep 18, 2019

It would be great if the dragStart event would not fire when the mouse goes down but rather when the cursor actually starts moving. Currently, dragStart and dragEnd will be fired if I only click/touch the slider without moving the cursor.

This would be useful to me because I am currently listening to both click events on items and dragStart/dragEnd Events and change styles accordingly. This leads to the item click having weird effects as it first transitions to the "drag" styles and back before the click event is fired.

I am currently using embla-carousel-react but I think this is something that concerns all variants of embla.

Let me know if you need an example.

Great Work by the way! 😊

@davidjerleke davidjerleke added good first issue Good for newcomers investigating Issue is being looked into labels Sep 18, 2019
@davidjerleke
Copy link
Owner

Hi Jeff (@Tscheffrey),

Thank you for opening this issue!

You have a good point 👍. Thinking about these event names, they are pretty unfortunate as you describe. Considering the current implementation and behaviour of dragStart and dragEnd they should be renamed like below to better represent what they actually do:

  • dragStart --> pointerDown
  • dragEnd --> pointerUp

If you don't mind you can post a CodeSandbox example for other users to understand this issue better.

👉 Suggestion

I agree with you that this needs attention. Maybe we should consider achieving something like this:

  • dragStart --> When pointer is down and has moved from its original position.

  • dragEnd --> When dragStart has fired and pointer is released.

  • pointerDown --> When pointer is down.

  • pointerUp --> When pointer is released.

What do you think? Does this make sense?

Best,
David

@Tscheffrey
Copy link
Author

Tscheffrey commented Sep 19, 2019

Hey David (@davidjerleke),

thank you for your quick reply. I'll make a CodeSandbox if I have some time today.

I totally agree with what you suggested, as the pointerDown / pointerUp event might also be useful for some users. This way, we can clearly differentiate between when the touch of the user starts and when the actual dragging occurs.

Suggestion

We might want to add threshold which will hold back the dragStart event if the finger of the user moves only slightly (e.g. less than 5px). That way, slight inaccuracies in the touchscreen will not negatively effect the user experience.

@davidjerleke
Copy link
Owner

davidjerleke commented Sep 21, 2019

Hello Jeff (@Tscheffrey),

Thank you for the feedback and your ideas 👍.
Sounds good. Maybe we could expose the drag distance as a parameter like so:

embla.on('dragStart', dragDistance => {
  // Do something here
})

And then it's up to the user to implement the threshold logic:

const myDragThreshold = 5

embla.on('dragStart', dragDistance => {
  if (dragDistance > myDragThreshold) {
    // Do something here 
  } 
})

What do you think?

Best,
David

@davidjerleke davidjerleke added the feature request New feature or request label Sep 21, 2019
@davidjerleke davidjerleke added the awaiting response Issue is awaiting feedback label Sep 28, 2019
@Tscheffrey
Copy link
Author

Tscheffrey commented Oct 2, 2019

Hi David (@davidjerleke),

I think this wouldn't work because as I understand, the dragStart event will only be fired once when the dragging starts. This means that there is no way of knowing when the threshold actually starts to get larger than what your threshold is. Also, there would be no way of preventing the actual dragging itself, even if you know that your threshold has not been exceeded yet.

The only way how this might work is to have another event that fires when dragging occurs, before the result is rendered to the screen, maybe called dragging. Then we could have something similar to e.preventDefault() with other events, in order to cancel the render.
That could look like this:

const myDragThreshold = 5

embla.on('dragging', e => {
  if (e.dragDistance < myDragThreshold) {
    e.preventDragging()
  } 
})

But I guess at this point, it might be easier to just have the drag threshold as a config parameter:

const embla = EmblaCarousel(emblaNode, {
  dragThreshold: 5,
  ...
})

Jeff

@davidjerleke davidjerleke removed the awaiting response Issue is awaiting feedback label Oct 2, 2019
@davidjerleke
Copy link
Owner

davidjerleke commented Oct 4, 2019

Hi Jeff (@Tscheffrey),

You're right. Of course, as you say dragStart will only fire once. What I forgot to mention is that Embla is already using a drag threshold today. So just by changing the dragStart behaviour to following suggestion:

When pointer is down and has moved from its original position

We will achieve both the threshold and the desired dragStart behaviour. If you think it makes sense for the user to be able to configure the drag threshold, would you mind opening an issue for this separately? Or do you want me to do it?

Thanks!

Kindly,
David

@Tscheffrey Tscheffrey reopened this Oct 4, 2019
@Tscheffrey
Copy link
Author

Tscheffrey commented Oct 4, 2019

Hi David (@davidjerleke),

What I forgot to mention is that Embla is already using a drag threshold today.

That is great! I imagine that the dragStart event should only be fired when the threshold is exceeded anyway, do you agree? I think that thats the most logical way of doing it.

I'll open up a ticket concerning the drag threshold configurability.

Jeff

@davidjerleke
Copy link
Owner

davidjerleke commented Oct 4, 2019

Hello Jeff (@Tscheffrey),

Thank you for your quick response. Cool, I hope I will have some free time to implement these features soon. Thanks for your patience.

Meanwhile, you could do a workaround by checking how far the pointer has moved from pointerdown to pointerup, and if it's more than let's say 5px ignore any click events. I really understand that this by no means is an optimal solution, it's just a suggestion to a temporary solution until these features are implemented.

I imagine that the dragStart event should only be fired when the threshold is exceeded anyway, do you agree?

I completely agree and think this makes sense 👍.

Thanks a lot for your ideas and taking time to discuss this.

All the best,
David

@Tscheffrey
Copy link
Author

Tscheffrey commented Oct 4, 2019

Hi David (@davidjerleke),

thanks for your responses and building this awesome slider! If I had the necessary skills, I'd help you implementing these features...

Meanwhile, you could do a workaround this by checking how far the pointer has moved from pointerdown to pointerup, and if it's more than let's say 5px ignore any click events.

Good idea, this would serve the purpose for now.

Greetings,
Jeff

@davidjerleke davidjerleke added upcoming A feature or bug fix is on its way for this issue and removed investigating Issue is being looked into labels Oct 4, 2019
@davidjerleke
Copy link
Owner

davidjerleke commented Nov 15, 2019

Hello Jeff (@Tscheffrey),

I'm working on this issue and would appreciate feedback from you. The issue discussed above with dragStart firing is in progress, but it's a breaking change so it will be released with v.3 as soon as possible. But I've implemented the click part you described:

This would be useful to me because I am currently listening to both click events on items and dragStart/dragEnd Events and change styles accordingly.

I've released a click check for Embla:

embla.clickAllowed()

Please read about it in the release description. You can see it in action in this CodeSandbox (it's also related to issue #24). This method is an opt-in feature and here is how it works: It returns false if:

Mouse Pointers

  • Mouse drags the carousel.

Touch Pointers

  • Touch drags the carousel.
  • Carousel is in motion (i.e. is scrolling).

Otherwise it will return true. I'd very much appreciate your feedback on this. Try v2.6.1 for this feature. Thank you in advance!

Kindly,
David

@davidjerleke davidjerleke added next major version and removed upcoming A feature or bug fix is on its way for this issue labels Jan 23, 2020
@davidjerleke
Copy link
Owner

davidjerleke commented Jun 1, 2020

I've taken the unfortunate naming into account and renamed the drag events to pointer events:

  • dragStart --> pointerDown
  • dragEnd --> pointerUp

This has been implemented and released with version 3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants