Turn off events.

[hemanth]

$('h1').on('click',function(){console.log('hi');}); 

$('h1').on('click',function(){console.log('yo');});

$('h1').off('click') // Turn off them all.

$('h1').off('click', $('h1').handlers[0]); // Will turn off the first one.

Similarity with nodeList.

[remy]

Can you restore the 2 white space - otherwise I see a 100% file change.

[hemanth]

Ooops!

[mathiasbynens]

@remy FYI you can also view any commit without whitespace differences by appending ?w=1 to the URL. E.g. https://github.com/remy/min.js/pull/8/files?w=1

[remy]

Yeah, I've been doing that - but integrating the change also means reversing out the white space changes - fun times :)

[remy]

Happy to merge this after my code review notes have been actioned.

I hope you don't see this as my being a pain - I'm trying to start giving decent feedback on pull requests that can help developers. In the past, I'll pulled the changes in, then made the fixes myself, which doesn't help the person who contributed the change, since they don't learn what changes we needed to land in the project.

I do appreciate the contribution :)

[hemanth]

@remy Not an issue at all! I have made the changes and pushed, please have a look and let me know if that is fine.

[remy]

Let me know when you're done with the changes. I can see some of them have come through, but the diff still shows 100% change due to spaces converted to tabs (I should really add a .jshint file to help with this), but also a few other bits - so I don't want to assume this is done whilst you're still working.

Equally, if you're done, I can point you to a few issues outstanding :)

[hemanth]

• White spaces. Rechecked with vim and sublime.
• Avoid self = this.
• Let the braces hug.
• this.handlers[forEach]
• Find other bits? Help!

I think I'm done.

[remy]

Hmm. Actually reviewing the code closer, there's a bigger issue. Basically the handler array should be specific to each element, because as it the array grows, in your off method, it's looping over everything.

I've written my own event handler code that also has an off method (in xui.js), so I'll see if I can merge these two together.

[hemanth]

@remy Oh ok! I was under the notation that this was bound to a particular element and hence it was specific. Thanks for the feedback 👍

[remy]

Hmm, actually you're right - I might have misread the change.

I think I need to pull it in, write some tests and see how it goes :)

On 25 May 2013 13:11, hemanth.hm notifications@github.com wrote:

@remy https://github.com/remy Oh ok! I was under the notation that thiswas bound to a particular element and hence it was specific. Thanks for the
feedback [image: 👍]

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-18446186
.

[hemanth]

Okies :)

Tried :

± |master ✗| → grunt test
Running "jasmine:compiled" (jasmine) task
Testing jasmine specs via phantom
.................
17 specs in 0.008s.
>> 0 failures

Done, without errors.

And also was playing around here I still feel that we need that self=this in element#off

[remy]

Yeah, you missed my point about removing self = this - I said we prefer to use bind, so the method should have been:

  element.off = function(event, fn) {
    if (!fn) {
      this.handlers[forEach](function(handler) {
        this.removeEventListener(event, handler, false);
      }.bind(this));
    } else {
      this.removeEventListener(event, fn, false);
    }
    return this;
  };

[remy]

I've got this all working on a branch now, but I'm now looking at why I original, intentionally left out an off method. The file is supposed to be plain and simple, but the new off code is increasing file size by 50%.

Not sure how anal I should be about a foot print less than 1K though...

[hemanth]

Oh ok! Even I was wondering why off was not present, at first thought of raising an issue, later tried this P.R.
But this trade off would be useful?

Binding this to the callback, /me misread it!

[remy]

I still have merged this pull request only because I wonder if it's worth trying to preserve the simplicity of the library - and not have an off method.

Then on the other hand, what does it matter if one more method is added?

I'd like to put this to a public vote. If there's a "decent" number of people who want this functionality, I'll add it in. By decent, I guess I mean 5 or so +1s (-1 obvious counts against).

Sound reasonable?

[hemanth]

@remy Yup, this seems to be the best way to decide conflicts like this 👍 :)

[tmaslen]

How about giving yourself a performance budget, so every time you add something you improve or take something out. Decide a maximum file size.

Not tried running the code, but nodelist and element seem to have identical methods assigned to them. Is it possible to do:

[
  nodelist,
  element
].forEach(function(obj) {
  obj.off = function() { ... }
  // etc
}

You could reduce the file size a bit then though the code would be harder to read. But if a goal of the project is to have an extremely small file size then it would be justified.

/t

[remy]

@maslen that's a nice idea for space saving, but not sure it works with this source code, since the nodeList and element methods are different (for on, off, etc).

Also it's more the logical size, of the api, rather than the file size. 500bytes compared 1k is no real difference unless it's 100% experiment, so I'm not hellbent on keeping the file size über small.

[zigotica]

to .off

another question: are events delegated? since .on accepts (event, fn) only I would say no, but i might've missed smth. wouldn't delegation be a must these days?

[remy]

@zigotica what do you mean by delegated? You can do it with regular code: http://jsbin.com/awikaz/1/edit

[zigotica]

yep, i ment sugar for that :)

[remy]

@zigotica like this: https://github.com/remy/min.js/blob/master/src/delegate.js :)

[zigotica]

grosso!
thanx

On Friday, June 28, 2013, Remy Sharp wrote:

@zigotica https://github.com/zigotica like this:
https://github.com/remy/min.js/blob/master/src/delegate.js :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-20206828
.

Sergi

zigotica.com
smeseguer.com
http://flickr.com/zigotica

[iamdustan]

👍 from me!

[ghost]

And one more 👍
I don't know if it's still being discussed, but I'll just add my vote

[remy]

It's because I prefer simplicity, and I feel like the library in it's
current state is actually quite good to learn from.

I'm not saying this change makes it hard and complex but I'm just
explaining why I'm still sitting on this change.
On 12 Jul 2013 15:19, "Webeled" notifications@github.com wrote:

And one more [image: 👍]
I don't know if it's still being discussed, but I'll just add my vote

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/8#issuecomment-20879152
.

[daniel-norman-sociomantic]

👍

[jslegers]

Why not move events into a separate "observer" library and keep min.js focused on DOM querying / manipulation?

[eugene-eeo]

+1 to @jslegers's suggestion

[brito]

👎 to increasing file size this way
👍 to @jslegers although this is only one way to deal with the event flow pattern

Another way is to have functions only respond to states reflected on the element:

<button disabled></button>

.on( '[disabled]', triggerQueueForDisabled)

👍 for adding something like denodify and coldcall instead

yours,
—Super random guy who just butted in the convo

[hueyy]

[garygreen]

👍 it's very common to disable events if you maintain a plugin and implement some kind of destroy method.

[lewisje]

👍 to support for removing event handlers

[tanaydin]

👍

[JoyceBabu]

👍

[karneaud]

maybe the on/ off/ one should be a plugin.... :-)