Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Added optional "data" argument to bind() and one() event methods #376

Closed
wants to merge 10 commits into from

6 participants

@ranbena

I worked on bringing the feature back. This is now possible:

$el.bind("click", {"foo": "bar"}, callback);
$el.one("click", {"foo": "baz"}, callback);

Had to really dig deep in the code to comprehend and modify.
Also, created a unit test page event.html to make sure the new functionality is solid ;)

@ollym

Yea event code is a mess. @mislav @madrobby would you agree that we move all event logic into the .on() function? This is what jQuery have done in 1.7 and makes maintenance soo much easier. I've been experimenting on a private fork and came up with this: https://github.com/ollym/yocto/blob/master/src/event.js#L11-38. Only minimal testing but provides the same functionality for delegate(), live() and bind(). See http://api.jquery.com/on/

I'm personally NOT in favour of doing this because it will get confused with the data bound to the event at the time of triggering. It's also much easier to pass data to your callback at the point of binding rather than triggering. Maybe it's a nice little tweak but if you do decide to add it, make sure the data goes as the last parameter so as not to break the API.

@mislav
Collaborator
@ranbena

Called it bindData so as to not conflict with trigger data (trigData)

@ranbena

I used your previous implementation (v0.5)

@ranbena

Had to put it in a non-anonymous function for the remove to work. It was pointing to fn before.

@ranbena

The data is preserved in handler

@ranbena

This is the only thing I'm unsure of - I'm assuming there's only one handler returned. Is that a safe assumption?

@ranbena

Hey guys, I put in some remarks to better understand my changes.

Of course when saying "I had to dig in", I had no intention of hinting at the code's tidiness. It's complex but well written and honestly I've learned much from it. (Evidence drove me a bit crazy though...)

@tmquinn

So how exactly is this supported? I see it's supported in the proxyfn, but I don't see where to pass it in...

@ranbena

Hey Quinn,

It's just like the jQuery syntax.
$el.bind(type, {"key":"value"}, function) (also applys to $el.one())

Example code

$el.bind("click", {"foo": "bar"}, function(e){
    alert(e.data.foo);
});

Take a look at the unit test test/event.html I created especially to test these out.

@tmquinn
@ranbena

Yeah yeah. It's not in the current master - only in the pull request.

@madrobby
Owner

Hi! I'd really like this to make it in—I tested your pull request against current master and it wouldn't automatically merge. After manually merging, I now get 10 test failures and 1 error when running the tests in zepto.html. Can you update this for the current master branch and move your tests to zepto.html?

@ranbena

On it

@ranbena

Ready.
I went ahead and added the new feature to live(), delegate() and on(). All have appropriate tests in zepto.html.

Check it out, tell me if it's cool.
Ran

@mislav mislav was assigned
@mislav
Collaborator

Seems like this patch has progressed nicely. However I won't pull it in yet, since I don't like how arguments are handled. Such code doesn't look very nice:

+  $.fn.on = function(event, arg2, arg3, arg4){
+    if (!arg2 || $.isFunction(arg2) || $.isObject(arg2))
+      return this.bind(event, arg2, arg3);
+    else
+      return this.delegate(arg2, event, arg3, arg4);

But @ranbena you don't have to sweat about it, since I can prettify arg handling. My only question is why did you save bindData to the internal handler object and later read the data from it, when you could have simply used the closure?

@martron

Has any progress been made in merging this to master? This would be very handy in an app I'm building.

Thanks

@madrobby
Owner

Any takers on bringing this up to speed and handling @mislav's change requests?

@mislav mislav referenced this pull request from a commit
@mislav mislav Ensure `.trigger()` does not set `event.data`
Extra arguments to `.trigger()` are extra arguments to pass to event
handlers, but they're *not* to be saved as `event.data`.

This is for jQuery compatibility.

References #376
2476c8e
@mislav mislav referenced this pull request from a commit
@mislav mislav Add `data` argument support for `.bind()` and `.on()`
The value of this argument will be copied to `event.data` property before
executing each event handler.

Closes #376
993ba78
@mislav
Collaborator

Hey @ranbena, thanks for all the work you did. Based on your changes I've made an alternate implementation, trying to avoid the normalizeArgs method and keeping the diff to event.js as small as possible. I've opened another PR for my implementation.

Ideally I wanted to implement this functionality as a plugin but it wouldn't be possible to hook into event handling so intimately from outside, so I think we'll ship this in Zepto core.

@ranbena

Wow that was a really long time ago!
I'll peek at your code. Hopefully I'll remember something :)

@mislav mislav referenced this pull request from a commit
@mislav mislav Rewrite event methods, provide `.one()` with delegation & `data` arg
For jQuery compatibility, `.one()` should also support CSS selector for
event delegation and the `data` argument, but this wasn't easy to add in
the previous implementation.

This rewrites all public event methods so that the core functionality is
encapsulated in `.on/off()` methods, and other (mostly legacy) methods
now invoke them instead of the other way around. This will make it
easier to remove the deprecated methods in the future.

References #376
1d95c87
@mislav
Collaborator

Closing this in favor of the referenced PR, in which I also completely rewrote .on/off() and related methods so that the deprecated methods will be easier to remove in the future

@mislav mislav closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 4, 2011
  1. @ranbena
Commits on Jan 17, 2012
  1. @ranbena

    merged

    ranbena authored
Commits on Jan 18, 2012
  1. @ranbena

    resolved conflict

    ranbena authored
  2. @ranbena

    Added optional "data" argument to bind() and one() event methods. Als…

    ranbena authored
    …o, created an event test page.
Commits on Mar 21, 2012
  1. @ranbena
  2. @ranbena

    Added optional "data" argument to bind, one, live, delegate and on me…

    ranbena authored
    …thods.
    
    Added appropriate tests
  3. @ranbena

    merged conflicts

    ranbena authored
  4. @ranbena
  5. @ranbena

    gitignore

    ranbena authored
  6. @ranbena
This page is out of date. Refresh to see the latest.
Showing with 95 additions and 24 deletions.
  1. +2 −1  .gitignore
  2. +6 −6 README.md
  3. +28 −17 src/event.js
  4. +59 −0 test/zepto.html
View
3  .gitignore
@@ -6,4 +6,5 @@ pkg
*.swp
docs/*
.jhw-cache
-.rbenv-version
+.rbenv-version/.project
+/.project
View
12 README.md
@@ -116,16 +116,16 @@ hasClass('classname') // returns true of first element has a classname set
toggleClass('classname'[, switch]) // adds/removes class, or adds/removes it when switch == true/false
toggleClass(function(index, existingClasses){ return ...; }) // adds/removes class from a method
-on(type, [selector,] function) // add event listener to elements
+on(type [, selector] [, data], function) // add event listener to elements
off(type, [selector,] function) // remove event listener from elements
-bind(type, function) // add an event listener (see below)
-one(type, function) // add an event listener that only fires once
+bind(type [,data], function) // add an event listener (see below)
+one(type [,data], function) // add an event listener that only fires once
unbind([type [, function]]) // remove event listeners
-delegate(selector, type, function) // add an event listener w/ event delegation (see below)
+delegate(selector, type [,data], function) // add an event listener w/ event delegation (see below)
undelegate(selector [, type[, function]]) // remove event listeners w/ event delegation
-live(type, function) // add an event listener that listens to the selector for current and future elements
+live(type [,data], function) // add an event listener that listens to the selector for current and future elements
die([, type[, function]]) // remove live listener
-trigger(type) // triggers an event
+trigger(type [,data]) // triggers an event
submit() // trigger form submit event
val() // returns the value of the form element
View
45 src/event.js
@@ -28,24 +28,29 @@
function matcherFor(ns) {
return new RegExp('(?:^| )' + ns.replace(' ', ' .* ?') + '(?: |$)');
}
+ function normalizeArgs(arg1, arg2){
+ return ($.isFunction(arg2)) ? {data: arg1, callback: arg2} : {callback: arg1};
+ }
function eachEvent(events, fn, iterator){
if ($.isObject(events)) $.each(events, iterator);
else events.split(/\s/).forEach(function(type){ iterator(type, fn) });
}
- function add(element, events, fn, selector, getDelegate, capture){
+ function add(element, events, bindData, fn, selector, getDelegate, capture){
capture = !!capture;
var id = zid(element), set = (handlers[id] || (handlers[id] = []));
eachEvent(events, fn, function(event, fn){
var delegate = getDelegate && getDelegate(fn, event),
callback = delegate || fn;
var proxyfn = function (event) {
- var result = callback.apply(element, [event].concat(event.data));
+ var handler = findHandlers(this, event.type, fn);
+ if (handler.length) event.data = handler[0].bindData;
+ var result = callback.apply(element, [event].concat(event.trigData));
if (result === false) event.preventDefault();
return result;
};
- var handler = $.extend(parse(event), {fn: fn, proxy: proxyfn, sel: selector, del: delegate, i: set.length});
+ var handler = $.extend(parse(event), {fn: fn, bindData: bindData, proxy: proxyfn, sel: selector, del: delegate, i: set.length});
set.push(handler);
element.addEventListener(handler.e, proxyfn, capture);
});
@@ -62,9 +67,10 @@
$.event = { add: add, remove: remove }
- $.fn.bind = function(event, callback){
+ $.fn.bind = function(event, arg2, arg3){
+ var args = normalizeArgs(arg2, arg3);
return this.each(function(){
- add(this, event, callback);
+ add(this, event, args.data, args.callback);
});
};
$.fn.unbind = function(event, callback){
@@ -72,9 +78,10 @@
remove(this, event, callback);
});
};
- $.fn.one = function(event, callback){
+ $.fn.one = function(event, arg2, arg3){
+ var args = normalizeArgs(arg2, arg3);
return this.each(function(i, element){
- add(this, event, callback, null, function(fn, type){
+ add(this, event, args.data, args.callback, null, function(fn, type){
return function(){
var result = fn.apply(element, arguments);
remove(element, type, fn);
@@ -115,8 +122,9 @@
}
}
- $.fn.delegate = function(selector, event, callback){
- var capture = false;
+ $.fn.delegate = function(selector, event, arg3, arg4){
+ var capture = false,
+ args = normalizeArgs(arg3, arg4);
if(event == 'blur' || event == 'focus'){
if($.iswebkit)
event = event == 'blur' ? 'focusout' : event == 'focus' ? 'focusin' : event;
@@ -125,7 +133,7 @@
}
return this.each(function(i, element){
- add(element, event, callback, selector, function(fn){
+ add(element, event, args.data, args.callback, selector, function(fn){
return function(e){
var evt, match = $(e.target).closest(selector, element).get(0);
if (match) {
@@ -142,8 +150,9 @@
});
}
- $.fn.live = function(event, callback){
- $(document.body).delegate(this.selector, event, callback);
+ $.fn.live = function(event, arg2, arg3){
+ var args = normalizeArgs(arg2, arg3);
+ $(document.body).delegate(this.selector, event, args.data, args.callback);
return this;
};
$.fn.die = function(event, callback){
@@ -151,9 +160,11 @@
return this;
};
- $.fn.on = function(event, selector, callback){
- return selector === undefined || $.isFunction(selector) ?
- this.bind(event, selector) : this.delegate(selector, event, callback);
+ $.fn.on = function(event, arg2, arg3, arg4){
+ if (!arg2 || $.isFunction(arg2) || $.isObject(arg2))
+ return this.bind(event, arg2, arg3);
+ else
+ return this.delegate(arg2, event, arg3, arg4);
};
$.fn.off = function(event, selector, callback){
return selector === undefined || $.isFunction(selector) ?
@@ -163,7 +174,7 @@
$.fn.trigger = function(event, data){
if (typeof event == 'string') event = $.Event(event);
fix(event);
- event.data = data;
+ event.trigData = data;
return this.each(function(){ this.dispatchEvent(event) });
};
@@ -173,7 +184,7 @@
var e, result;
this.each(function(i, element){
e = createProxy(typeof event == 'string' ? $.Event(event) : event);
- e.data = data; e.target = element;
+ e.trigData = data; e.target = element;
$.each(findHandlers(element, event.type || event), function(i, handler){
result = handler.proxy(e);
if (e.isImmediatePropagationStopped()) return false;
View
59 test/zepto.html
@@ -1697,6 +1697,47 @@
t.assert(prevented);
},
+
+ testBindData: function (t) {
+ var eventData;
+
+ var handler = function (e) {
+ eventData = e.data;
+ };
+
+ // bind
+ $('#some_element')
+ .bind('click', {customKey: 10}, handler)
+ .trigger('click');
+
+ t.assertEqual(eventData.customKey, 10);
+
+
+ // one
+ $('#another_element')
+ .one('click', {customKey: 20}, handler)
+ .trigger('click');
+
+ t.assertEqual(eventData.customKey, 20);
+
+ // on
+ $('p').on('click', 'span.yay', {customKey: 30}, handler);
+ $('span.yay').trigger('click');
+
+ t.assertEqual(eventData.customKey, 30);
+
+ // live
+ $('body p').live('click', {customKey: 40}, handler);
+ click($('p').get(0));
+
+ t.assertEqual(eventData.customKey, 40);
+
+ // delegate
+ $('div#delegate_test').delegate('span.second-level', 'click', {customKey: 50}, handler);
+ click($('span.second-level').get(0));
+ t.assertEqual(eventData.customKey, 50);
+
+ },
testCreateEventObject: function(t){
var e = $.Event('custom');
@@ -1767,6 +1808,24 @@
t.assertIdentical(2, $(form).triggerHandler('submit'));
t.assertEqual('1 2', executed.join(' '));
},
+
+ testBindDataAndTriggerEventObject: function (t) {
+ var eventData, triggerData;
+
+ var handler = function (e, data) {
+ eventData = e.data;
+ triggerData = data;
+ };
+
+ $('#some_element')
+ .bind('click', {customKey: 10}, handler)
+ .trigger('click', {customKey: 20});
+
+ t.assert(eventData);
+ t.assertEqual(eventData.customKey, 10);
+ t.assert(triggerData);
+ t.assertEqual(triggerData.customKey, 20);
+ },
testUnbind: function(t){
var counter = 0, el = $('#another_element').get(0);
Something went wrong with that request. Please try again.