html-next / ember-hammertime

TouchAction (aka "fastclick") Support for Ember Applications
MIT License
56 stars 23 forks source link

Don't apply hammer-time to selects, or whitelist #11

Closed wongpeiyi closed 8 years ago

wongpeiyi commented 8 years ago

Following up from https://github.com/hammerjs/hammer-time/issues/28:

Since the polyfill doesn't work on selects on mobile, perhaps the AST walker should not apply touch-action to select tags? Alternatively, are you open to some kind of config to whitelist the elements instead of hard-coding isFocusable?

runspired commented 8 years ago

I agree with updating isFocusable, but am additionally open to exposing a whitelist and blacklist API for this.

wongpeiyi commented 8 years ago

Thanks, created PR to address the first for now.

I think I could add a whitelist / blacklist for the components by allowing extending the initializer, but I'm not familiar with htmlbars plugins – is there a good way to pass in config?

wongpeiyi commented 8 years ago

@runspired on exposing an API, could you let me know what you think of https://github.com/wongpeiyi/ember-hammertime/commit/44e5da471d4716f3737a6957fbea193e7956f91a?w=1?

I'd like to expose touchActionSelectors to define which selectors get the touch-action style applied, and also touchActionProperties to define the actual CSS style that gets applied.

I find applying cursor: pointer to everything too restrictive – for example if I have a responsive app I would want to apply it manually under the mobile breakpoint, otherwise my input elements would have cursor: pointer applied even on desktop.

We can expose these properties for Ember components easily enough by letting them be overridden. The AST walker is more tricky though – the only way to expose configuration I could find was in https://github.com/ember-cli/ember-cli-htmlbars/blob/864b02ed739adddc42af27a7f80177043d4e9c1e/ember-addon-main.js#L48-L50

runspired commented 8 years ago

I have inline code comments but can't without a PR, I want to proceed with this but with a couple of alterations, one of which being both the mixin and the AST walker should pull from config.

(EDIT feel free to open a PR and we'll work on this from there)

wongpeiyi commented 8 years ago

Thanks, just wanted to be sure it was in the right direction first – have opened a PR.

wongpeiyi commented 8 years ago

@runspired sorry for the bump, just want to be sure you didn't miss it previously – waiting for your comments on https://github.com/runspired/ember-hammertime/pull/13

wongpeiyi commented 8 years ago

Thanks for the merges

Resolved in: https://github.com/runspired/ember-hammertime/pull/12 https://github.com/runspired/ember-hammertime/pull/13