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 issue #1152] Enhance event-flick module with flickleft, flickright, flickup, flickdown sugar events #1323

Open
wants to merge 6 commits into
base: dev-3.x
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 125 additions & 45 deletions src/event-gestures/js/Flick.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,8 @@ var GESTURE_MAP = Y.Event._GESTURE_MAP,
_FLICK_START_HANDLE = "_fsh",
_FLICK_END_HANDLE = "_feh",
_FLICK_MOVE_HANDLE = "_fmh",

NODE_TYPE = "nodeType";
NODE_TYPE = "nodeType",
config;

/**
* Sets up a "flick" event, that is fired whenever the user initiates a flick gesture on the node
Expand All @@ -68,7 +68,7 @@ var GESTURE_MAP = Y.Event._GESTURE_MAP,
* @param fn {function} The method the event invokes. It receives an event facade with an e.flick object containing the flick related properties: e.flick.time, e.flick.distance, e.flick.velocity and e.flick.axis, e.flick.start.
* @param cfg {Object} Optional. An object which specifies any of the following:
* <dl>
* <dt>minDistance (in pixels, defaults to 10)</dt>
* <dt>minDistance (in pixels, defaults to 100)</dt>
* <dd>The minimum distance between start and end points, which would qualify the gesture as a flick.</dd>
* <dt>minVelocity (in pixels/ms, defaults to 0)</dt>
* <dd>The minimum velocity which would qualify the gesture as a flick.</dd>
Expand All @@ -81,18 +81,18 @@ var GESTURE_MAP = Y.Event._GESTURE_MAP,
* </dl>
* @return {EventHandle} the detach handle
*/

Y.Event.define('flick', {
config = {
_eventType: "flick",

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

self._onStart(e, node, subscriber, ce);
});
subscriber[_FLICK_START_HANDLE] = startHandle;
},

Expand All @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good point.

var extra = args[3] || {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// remove the extra arguments from the array as specified by
// http://yuilibrary.com/yui/docs/event/synths.html
args.splice(3,1);

if (!(MIN_VELOCITY in params)) {
params[MIN_VELOCITY] = this.MIN_VELOCITY;
}
if (extra && !extra[MIN_DISTANCE]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

extra[MIN_DISTANCE] = this.MIN_DISTANCE;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Relying on this seems brittle in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How so?

}

if (!(MIN_DISTANCE in params)) {
params[MIN_DISTANCE] = this.MIN_DISTANCE;
}
if (extra && !extra[MIN_VELOCITY]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

extra[MIN_VELOCITY] = this.MIN_VELOCITY;
}

if (!(PREVENT_DEFAULT in params)) {
params[PREVENT_DEFAULT] = this.PREVENT_DEFAULT;
}
if (extra && !extra[PREVENT_DEFAULT]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

extra[PREVENT_DEFAULT] = this.PREVENT_DEFAULT;
}

return params;
return extra;
}
},

_onStart: function(e, node, subscriber, ce) {

var start = true, // always true for mouse
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

endHandle,
moveHandle,
doc,
preventDefault = subscriber._extra.preventDefault,
origE = e;
preventDefault = subscriber._extra[PREVENT_DEFAULT],
origE = e,
self = this;

if (e.touches) {
start = (e.touches.length === 1);
Expand All @@ -163,7 +168,9 @@ Y.Event.define('flick', {

doc = (node.get(NODE_TYPE) === 9) ? node : node.get(OWNER_DOCUMENT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code comment would be good here.

if (!endHandle) {
endHandle = doc.on(EVENT[END], Y.bind(this._onEnd, this), null, node, subscriber, ce);
endHandle = doc.on(EVENT[END], function (e) {
self._onEnd(e, node, subscriber, ce);
}, { standAlone: true });
subscriber[_FLICK_END_HANDLE] = endHandle;
}

Expand Down Expand Up @@ -235,33 +242,106 @@ Y.Event.define('flick', {
if (params.axis) {
axis = params.axis;
} else {
axis = (Math.abs(xyDistance[0]) >= Math.abs(xyDistance[1])) ? 'x' : 'y';
axis = (Math.abs(xyDistance[0]) >= Math.abs(xyDistance[1])) ? "x" : "y";
}

distance = xyDistance[(axis === 'x') ? 0 : 1];
distance = xyDistance[(axis === "x") ? 0 : 1];
velocity = (time !== 0) ? distance/time : 0;

if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) && (Math.abs(velocity) >= params[MIN_VELOCITY])) {
this._fireEvent(e, time, distance, velocity, axis, start, params, ce);
subscriber[_FLICK_START] = null;
}
}
},

e.type = "flick";
e.flick = {
time:time,
distance: distance,
velocity:velocity,
axis: axis,
start : start
};
_fireEvent: function (e, time, distance, velocity, axis, start, params, ce) {
if (this._isValidFlick(velocity, distance, params, axis)) {

ce.fire(e);
e.type = this._eventType;
e.flick = {
time:time,
distance: distance,
velocity:velocity,
axis: axis,
start : start
};

}
ce.fire(e);
}
},

subscriber[_FLICK_START] = null;
}
_isValidFlick: function (velocity, distance, params, axis) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

MIN_VELOCITY : 0,
MIN_DISTANCE : 0,
MIN_DISTANCE : 100,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why 100 pixels?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

PREVENT_DEFAULT : false
});
};


Y.Event.define('flick', config);
Y.Event.define('flickleft', Y.merge(config, {
_eventType: 'flickleft',
_isValidFlick: function (velocity, distance, params, axis) {
if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) &&
(Math.abs(velocity) >= params[MIN_VELOCITY]) &&
axis === 'x' && distance < 0) {

return true;
}
else {
return false;
}
}
}));

Y.Event.define('flickright', Y.merge(config, {
_eventType: 'flickright',
_isValidFlick: function (velocity, distance, params, axis) {
if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) &&
(Math.abs(velocity) >= params[MIN_VELOCITY]) &&
axis === 'x' && distance > 0) {

return true;
}
else {
return false;
}
}
}));

Y.Event.define('flickup', Y.merge(config, {
_eventType: 'flickup',
_isValidFlick: function (velocity, distance, params, axis) {
if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) &&
(Math.abs(velocity) >= params[MIN_VELOCITY]) &&
axis === 'y' && distance < 0) {

return true;
}
else {
return false;
}
}
}));

Y.Event.define('flickdown', Y.merge(config, {
_eventType: 'flickdown',
_isValidFlick: function (velocity, distance, params, axis) {
if (isFinite(velocity) && (Math.abs(distance) >= params[MIN_DISTANCE]) &&
(Math.abs(velocity) >= params[MIN_VELOCITY]) &&
axis === 'y' && distance > 0) {

return true;
}
else {
return false;
}
}
}));
118 changes: 118 additions & 0 deletions src/event-gestures/tests/manual/flick-simple.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1.0" />
<title>event-flick simple manual test</title>
<link rel="stylesheet" href="http://yui.yahooapis.com/pure/0.3.0/pure-min.css">
<style>
body {
font-family: sans-serif;
}
h1 {
margin-bottom: 0;
}
h2 {
font-weight: normal;
font-size: 20px;
margin: 0;
}

#flickContainer, #flickContainer2, #flickContainer3 {
text-align: center;
height: 50%;
color: white;
padding: 40px 0;
}

#flickContainer {
background: #42d692;
}

#flickContainer2 {
background: rgb(253, 122, 2);
}

#flickContainer3 {
background: blue;
}


#clearBtn {
padding: 20px 30px;
font-size: 160%;
border-radius: 50px;
text-transform: uppercase;
letter-spacing: 0.1em;

}
#log, #log2 {
font-family: monospace;
font-size: 18px;
color: black;
text-align: center;
list-style: none;
}
</style>
</head>
<body>
<button class="pure-button pure-button-primary" id="clearBtn">Clear log</button>
<div class="pure-g-r">
<div class="pure-u-1-2">
<div id="flickContainer">

<h1>Flick!</h1>
<h2 id="flickDirection">Basic Flick with no options</h2>
<div id="log"></div>
</div>
</div>
<div class="pure-u-1-2">
<div id="flickContainer2">

<h1>Flick!</h1>
<h2 id="flickDirection">Flick with minDistance = 50, minVelocity = 1 px/s, preventDefault = true</h2>
<div id="log2"></div>
</div>
</div>
</div>



<script src="../../../../build/yui/yui.js"></script>


<script>
YUI({
filter: 'raw',
combine: false
}).use('event-flick', 'node', function (Y) {

var container = Y.one('#flickContainer'),
container2 = Y.one('#flickContainer2'),
h2 = Y.one('#flickDirection'),
log = Y.one('#log'),
log2 = Y.one('#log2'),
clearBtn = Y.one('#clearBtn');

clearBtn.on('click', function (e) {
log.set('innerHTML', '');
log2.set('innerHTML', '');
});



container.on(['flick', 'flickright', 'flickleft', 'flickup', 'flickdown'], function (e) {
log.appendChild('<li><strong>' + e.type + '</strong> with distance: ' + e.flick.distance + ' and velocity: ' + e.flick.velocity + '</li>');
});

container2.on(['flick', 'flickright', 'flickleft', 'flickup', 'flickdown'], function (e) {
log2.appendChild('<li><strong>' + e.type + '</strong> with distance: ' + e.flick.distance + ' and velocity: ' + e.flick.velocity + '</li>');
}, {minDistance: 50, minVelocity: 1, preventDefault: true});




});
</script>
</body>
</html>
Loading