Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

[Fix issue #1152] Enhance event-flick module with flickleft, flickright, flickup, flickdown sugar events #1323

Open
wants to merge 6 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

tilomitra commented Oct 15, 2013

This pull request fixes issue #1152 by enhancing the event-flick module.

What's in this pull request?

flick* events

Added 4 new synthetic "sugar" events that developers can hook on to:

  • flickleft
  • flickright
  • flickup
  • flickdown

These events have the exact same public API as the generic flick event, but fire only when the axis and direction properties match up to the respective directions.

Bit of code cleanup

I separated out the monolithic _onEnd() callback into smaller functions:

  • _onEnd()
  • _isValidFlick()
  • _fireEvent()

This helped me re-use a large portion of the CustomEvent config object when defining the new flick* events. These new events only overwrite the _isValidFlick() method. Check out

In addition to that, there was some minor code cleanup to make the code more readable.

minDistance default value change

I made a change in the flick event's minDistance property. By default, its value is now 100, so that it catches a legitimate flick, instead of a slight move of the finger. I believe it was 10 before.

Improve Test Coverage

event-flick did not have great tests. There were only 3 of them and they probably weren't working correctly because the assertions were inside a custom fire() method. If fire() wasn't called, the tests would pass anyway, since an empty test with no assertions passes. I improved the way the tests are written and added a bunch of new tests. event-flick has ~98% line coverage in all browsers. I'm not using event-simulate so these tests are very stable.

I also added a new manual test (flick-simple.html).

Tested on:

  • IE7-10
  • Latest Chrome/Safari/FF
  • iOS 7
  • Android 4.0.3

If someone has an Android device running on a newer Android version, it would be great if you can help me test this. No need to pull it down, just ping me and I can give you a URL that you can hit on your browser to run through the unit and manual tests.

TODO before merging

This is so I don't forget.

  • Update event-flick documentation
  • Push build files

tilomitra added some commits Oct 15, 2013

event-flick: Fix issue #1152
This commit to event-flick enhances the module by adding additional
sugar events: `flickleft`, `flickright`, `flickup`, and `flickdown`.

In addition, I have broken out some of the large monolithic functions
into smaller chunks for easier testing.

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
- subscriber[_FLICK_START] = null;
- }
+ _isValidFlick: function (velocity, distance, params, axis) {
@ericf

ericf Oct 15, 2013

Owner

This logic exists 5 times! Sounds like it needs to be a function that's reusable to me.

@tilomitra

tilomitra Oct 15, 2013

Contributor

It's not the same logic though. The if() statement in each implementation is different. The axis and direction checks change based on the type of event that is being subscribed to. How should I break this out more?

@ericf

ericf Oct 15, 2013

Owner

Also code comments and docs on what it means to be valid.

@tilomitra

tilomitra Oct 15, 2013

Contributor

I suppose I could split out this chunk into another function since it's shared:

if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) &&
(Math.abs(velocity)  >= params[MIN_VELOCITY])) {
  return true;
}
else {
  return false;
}
@ericf

ericf Oct 15, 2013

Owner

There's two checks:

  1. Is the flick valid
  2. Was there a specific direction

The first thing is repeated 5 times, it can be written once and called from the other places. Even better would be to simply check the axis first since that's an easier comparison before calling into the _isValidFlick() shared function.

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
}
},
MIN_VELOCITY : 0,
- MIN_DISTANCE : 0,
+ MIN_DISTANCE : 100,
@ericf

ericf Oct 15, 2013

Owner

Why 100 pixels?

@tilomitra

tilomitra Oct 16, 2013

Contributor

I'm changing this back to 10. That's what the documentation says (even though it's 0 here for some reason). HammerJS also uses 10. As long as it's not 0, I'm ok with it.

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
on: function (node, subscriber, ce) {
- var startHandle = node.on(EVENT[START],
- this._onStart,
- this,
- node,
- subscriber,
- ce);
-
+ //Using this instead of Y.bind() here because I find
+ //the order of arguments easier to read in this case
+ //since the callback gets the DOMEventFacade as it's first arg.
+ var self = this;
+ var startHandle = node.on(EVENT[START], function (e) {
@ericf

ericf Oct 15, 2013

Owner

The rest of this file uses one var declaration per scope.

@tilomitra

tilomitra Oct 15, 2013

Contributor

Good catch!

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
@@ -112,32 +112,37 @@ Y.Event.define('flick', {
}
},
- processArgs: function(args) {
- var params = (args.length > 3) ? Y.merge(args.splice(3, 1)[0]) : {};
+ processArgs: function(args, delegate) {
+ if (!delegate) {
@ericf

ericf Oct 15, 2013

Owner

Why not early return in this case instead of wrapping the while fn body in an if?

@tilomitra

tilomitra Oct 15, 2013

Contributor

Yes, good point.

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
@@ -112,32 +112,37 @@ Y.Event.define('flick', {
}
},
- processArgs: function(args) {
- var params = (args.length > 3) ? Y.merge(args.splice(3, 1)[0]) : {};
+ processArgs: function(args, delegate) {
+ if (!delegate) {
+ var extra = args[3] || {};
@ericf

ericf Oct 15, 2013

Owner

Seems like you should create a shallow copy with Y.merge() if you're going to mutate someone else's object.

@tilomitra

tilomitra Oct 15, 2013

Contributor

@ericf I was just following the guidelines provided here: http://yuilibrary.com/yui/docs/event/synths.html#extra-arguments

@ericf

ericf Oct 15, 2013

Owner

Yes, but you're mutating the extra, which someone could have saved a reference to and plans to use later, in which case you've mutated their object.

@ericf ericf commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
- if (!(MIN_VELOCITY in params)) {
- params[MIN_VELOCITY] = this.MIN_VELOCITY;
- }
+ if (extra && !extra[MIN_DISTANCE]) {
@ericf

ericf Oct 15, 2013

Owner

The first condition will always be true because you set it to default to a new object above.

@ericf

ericf Oct 15, 2013

Owner

This fails if someone sets this to 0 since it would be considered falsy.

@ericf ericf commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
- if (!(MIN_DISTANCE in params)) {
- params[MIN_DISTANCE] = this.MIN_DISTANCE;
- }
+ if (extra && !extra[MIN_VELOCITY]) {

@ericf ericf commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
- if (!(PREVENT_DEFAULT in params)) {
- params[PREVENT_DEFAULT] = this.PREVENT_DEFAULT;
- }
+ if (extra && !extra[PREVENT_DEFAULT]) {

@ericf ericf and 1 other commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
- if (!(MIN_VELOCITY in params)) {
- params[MIN_VELOCITY] = this.MIN_VELOCITY;
- }
+ if (extra && !extra[MIN_DISTANCE]) {
+ extra[MIN_DISTANCE] = this.MIN_DISTANCE;
@ericf

ericf Oct 15, 2013

Owner

Relying on this seems brittle in this case.

@tilomitra

tilomitra Oct 15, 2013

Contributor

How so?

@ericf ericf commented on the diff Oct 15, 2013

src/event-gestures/js/Flick.js
- return params;
+ return extra;
+ }
},
_onStart: function(e, node, subscriber, ce) {
var start = true, // always true for mouse
@ericf

ericf Oct 15, 2013

Owner

I don't understand this code comment. When is this not true?

@tilomitra

tilomitra Oct 15, 2013

Contributor

I didn't modify/originally write that, but it looks like that would be false if e.touches.length > 1.

@tilomitra

tilomitra Oct 16, 2013

Contributor

I added a unit test for the falsy case (when there is more than 1 finger on the screen) just to be certain and that test passes. Also added some code comments.

@ericf ericf commented on an outdated diff Oct 15, 2013

src/event-gestures/js/Flick.js
@@ -163,7 +168,9 @@ Y.Event.define('flick', {
doc = (node.get(NODE_TYPE) === 9) ? node : node.get(OWNER_DOCUMENT);
@ericf

ericf Oct 15, 2013

Owner

Code comment would be good here.

Contributor

tilomitra commented Oct 15, 2013

Looks like something failed downstream in Scrollview. I'll investigate this and report back.

Update: The fix that I made to processArgs() seemed to fix the failing test in Scrollview. It wasn't picking up that preventDefault = false, but it does now. All Scrollview unit tests pass and the manual tests work too.

tilomitra added some commits Oct 16, 2013

event-flick: Add necessary fixes as recommended by @ericf in
#1323

* Improve `processArgs()` method
* Add code comments all-round
* Separate out `_isValidFlick()` into `_isValidFlick()` and
`_isValidAxis()`. The `flick*` events over-write the latter.
Contributor

triptych commented Jun 3, 2014

@tilomitra if you import Hammer.js would that render this PR out of date?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment