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

Add touchstart listener if touchstart event is supported else add click listener. but do not add both. #1711

Merged
merged 2 commits into from
Sep 28, 2017

Conversation

sferoze
Copy link
Contributor

@sferoze sferoze commented Sep 21, 2017

Add touchstart listener if touchstart event is supported else add click listener. but do not add both.

@sferoze
Copy link
Contributor Author

sferoze commented Sep 24, 2017

@jhchen

I was thinking about this and I am wondering something. Is there ever a case where touchstart is supported by a device, and also mousedown is supported?

Maybe like the Surface? Which has a touch screen but also uses a mouse. I am not sure how that device handles it, but if touchstart is supported, you have to touch the screen to check the box. As the click listener is not added.

Maybe this is more appropriate?

domNode.addEventListener('mousedown', listEventHandler);
domNode.addEventListener('touchstart', listEventHandler);

If we use mousedown instead of the click event, it will not be called twice as the mouse and touch events are completely seperate.

Is this a better route or is the commit I have the better route? All depends on how something like the surface handles this... which I am not sure about.

@jhchen
Copy link
Member

jhchen commented Sep 25, 2017

Yes I suppose any device with an external keyboard would have this. Let's go with the mousedown then?

@sferoze
Copy link
Contributor Author

sferoze commented Sep 27, 2017

@jhchen I made a new commit with both mousedown and touchstart event added

@jhchen jhchen merged commit 098bb73 into slab:develop Sep 28, 2017
@jhchen
Copy link
Member

jhchen commented Sep 28, 2017

Thanks!

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.

2 participants