-
Notifications
You must be signed in to change notification settings - Fork 110
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
custom events on document & window #38
Conversation
Conflicts: src/bean.js
@@ -322,7 +359,6 @@ sink('remove', function (test, ok) { | |||
ok(true, 'remove all events 2'); | |||
}; | |||
bean.add(el, 'click.foo', handler1); | |||
bean.add(el, 'click.foo', handler1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed this because it's unfair to IE8 and below, it'll fail every time because of the way events propagate; plus the plain remove()
test preceding this test doesn't do the same thing even though the test is basically the same.
If the intention is to be able to remove additional handlers from within a handler for that same event then that should be a separate test because IE8 and below have some problems in that area that'll need special attention.
btw, this does nothing for cross-frame events, the check is for |
lol. it has to be in the actual commit message, not the pull request. but that's alright. also, it appears @fat has dropped from the planet. i'll go ahead and merge this. looks good |
woot for |
i didn't drop from the planet - just wan't crazy about the implementation of this proposal :/ |
Then speak up son! what don't you like? I'm happy to modify if I know what the issues are. Is it the fact that we're attaching yet another object to the handler (ref #19)? I've been pondering that one and wondering what your preferred alternative is. |
This is fine for now - I think @dperini's solution is the most elegant in nwevents and eventually something I'd prefer to move towards. I am just worried that this sort of thing will pile up over time and I don't think it's a good practice. no big deal though -- sorry for being so busy lately |
Challenge accepted |
adding, removing and modifying DOM element properties can generate unwanted "Mutation" events and in some browser it could also trigger unwanted reflow/repaint of the page. Avoiding those changes will help with performances. |
It occurred to me that for IE8 and below, that don't support propertychange on
document
orwindow
we should just remap the events todocumentElement
and the world will be at peace.closes #29
closes #30
closes #32
(does auto-close work if I put those in here? or do they have to be on the actual commit message?)
Is it more tempting to merge when I can close 4 issues at once?