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

ISSUE #73 Use touchTap event instead of touchstart. #75

Merged
merged 1 commit into from
Oct 15, 2015

Conversation

0xjocke
Copy link
Contributor

@0xjocke 0xjocke commented Oct 14, 2015

So we need a tapEvent since react doesnt support it (yet). It's possible to do a custom solution that saves touchStarts position and a timestamp. Then onTouchEnd compare that position and check how long time that has passed. This way we can ignore scrolling and "long touches".

A faster and more clean solution is the react-tap-event-plugin. It hooks into reacts event system and adds a touchTap event.
Then when react adds touchTap event its easy to remove this dependency.

From react-tap-event-plugin:
You've probably heard of iOS's dreaded 300ms tap delay. React's onClick attribute falls prey to it. Facebook's working on a solution in the form of TapEventPlugin, but it won't be made available until 1.0.

If you're reading this, you're probably working on a project that can't wait until they figure out how they want to publish it. This repo is for you.

When Facebook solves #436 and #1170, this repo will disappear.

@0xjocke
Copy link
Contributor Author

0xjocke commented Oct 14, 2015

If you want to try it out on a phone you could change server.js line 9 to:
}).listen(3000, '', function(err, result) {
instead of:
}).listen(3000, 'localhost', function(err, result) {

This way you can surf to your ip instead of using localhost. This is also possible to do from your phone as long as you are on the same network :)

@moroshko
Copy link
Owner

Thanks a lot!

moroshko added a commit that referenced this pull request Oct 15, 2015
ISSUE #73 Use touchTap event instead of touchstart.
@moroshko moroshko merged commit eb9c494 into moroshko:master Oct 15, 2015
@moroshko
Copy link
Owner

@bachstatter Including react-tap-event-plugin in react-autosuggest presents a challenge: If another 3rd party component or the app itself uses react-tap-event-plugin, we'll get:

Uncaught Error: Invariant Violation: EventPluginRegistry: Cannot inject two different event plugins using the same name, TapEventPlugin.

See: zilverline/react-tap-event-plugin#47

What are your thoughts about not including react-tap-event-plugin as part of react-autosuggest, and make it clear in the readme that it needs to be included manually to support mobile devices? Alternatively, we could add a config option in react-autosuggest to conditionally include react-tap-event-plugin. Or, maybe you have a better idea?

@0xjocke
Copy link
Contributor Author

0xjocke commented Oct 20, 2015

Thats an interesting challenge. I'm not sure if the best solution is to add a config option or just some instructions in the readme.

I found a third solution, we can find all events in the EventPluginRegistry. This if statement is false before injectTapEventPlugin() is invoked and true after. I'm not sure how reliable this is, but it seems to work.

import { registrationNameModules } from 'react/lib/EventPluginRegistry';

if (!registrationNameModules.onTouchTap) {
  injectTapEventPlugin();
}

What do you think? We could wait and see if zilverline respond to your issue.

@moroshko
Copy link
Owner

@bachstatter Checking registrationNameModules.onTouchTap is problematic, I think. If some other library is doing something with onTouchTap, and it run first, our code will do nothing. Or, if react-autosuggest runs first, we'll break the other plugin functionality.

@0xjocke
Copy link
Contributor Author

0xjocke commented Oct 21, 2015

The first case should be fine, since if onTouchTap is injected by some other library we wouldn't need to do it. But it might break other projected trying to use onTouchTap.

We could use https://github.com/JedWatson/react-tappable and wrap each item with the a Tappable component.

I'm not sure which solution is best...

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