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: Close datepicker on touchstart #1924

Merged
merged 2 commits into from
Jun 22, 2016
Merged

Fix: Close datepicker on touchstart #1924

merged 2 commits into from
Jun 22, 2016

Conversation

clemens
Copy link

@clemens clemens commented Jun 20, 2016

It seems that when the event system was reworked, the bug closed in #223 got reintroduced. This PR fixes this.

@acrobat
Copy link
Member

acrobat commented Jun 20, 2016

Can you add tests to avoid regressions like this in the future? Thanks!

@clemens
Copy link
Author

clemens commented Jun 21, 2016

I've never done this for JS so I hope it's OK like this: I just copied the test that's used for the corresponding mouse event to a separate directory and let it trigger a touchstart event instead of mousedown.

Let me know.

@acrobat
Copy link
Member

acrobat commented Jun 22, 2016

Looks good to me! @vsn4ik @Azaret do you guys have any comments on this?

@acrobat acrobat added this to the 1.7.0 milestone Jun 22, 2016
@vsn4ik
Copy link
Member

vsn4ik commented Jun 22, 2016

LGTM!

@acrobat
Copy link
Member

acrobat commented Jun 22, 2016

Thanks @clemens!

@acrobat acrobat merged commit 27e0ed1 into uxsolutions:master Jun 22, 2016
@acrobat
Copy link
Member

acrobat commented Jun 22, 2016

Hmm I've just merged but this pr came to my attention #1284, are we also introducing the same bug again @clemens?

@clemens clemens deleted the close-datepicker-on-touchstart branch June 22, 2016 13:21
@clemens
Copy link
Author

clemens commented Jun 22, 2016

That's quite likely. However: Mousedown wasn't triggered for me despite not scrolling – hence the PR.

I'll double check and get back to you.

@acrobat
Copy link
Member

acrobat commented Jun 22, 2016

That's great! Or if you have/find another workaround to support both cases would be nice!

@clemens
Copy link
Author

clemens commented Jun 26, 2016

I've done a little investigation and it looks like there's no easy way to get both things working properly. The best idea I have (inspired by things I think I remember from other libs) would be as follows:

  • capture the origin of a touchstart event
  • trigger the closing action on mouseup and touchend instead of mousedown and touchstart (the mouseup change is merely for consistency because it's the equivalent of touchend)
  • cancel the touchend handling if the distance between the origin captured in touchstart and the destination in touchend is below a certain (user-editable) threshold (i.e. it counts as a click rather than a scroll below that threshold)

I'm afraid my JS skills probably don't suffice for a decent implementation of that solution.

For the time being, I'd argue that being able to close the datepicker by touching outside of it is more important than the datepicker staying open when the user scrolls – but maybe that's just my point of view. I'd understand if you want to revert my PR.

Cheers,

  • C.

@acrobat acrobat mentioned this pull request May 1, 2017
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.

3 participants