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

Issue 650 #753

Merged
merged 3 commits into from Mar 12, 2013

Conversation

Projects
None yet
5 participants
@jagill
Contributor

jagill commented Feb 26, 2013

This is a fix for issue #650. Event handlers can now be set multiple times, without clobbering the previous definitions.

I couldn't find an easy way within meteor itself to test these changes, but I've tested them with meteor-mocha-web. Clone https://github.com/mad-eye/meteor-eventhandlers.git and run runme (you might have to mrt update first).

Once this gets reviewed, I'll update the Spark documentation.

@@ -79,7 +79,11 @@
events: function (eventMap) {
var events =
(this._tmpl_data.events = (this._tmpl_data.events || {}));
_.extend(events, eventMap);
console.log("Events:", events);

This comment has been minimized.

@glasser

glasser Mar 5, 2013

Member

Remove this log.

This comment has been minimized.

@jagill

jagill Mar 5, 2013

Contributor

Ooops... sorry, that was sloppy of me.

@glasser

This comment has been minimized.

Member

glasser commented Mar 5, 2013

Can you add a test of this behavior in templating/templating_tests.js, eg to the templating - events test?

@gschmidt

This comment has been minimized.

Member

gschmidt commented Mar 5, 2013

James, thanks, this looks great. @dgreensp and I were wanting something like this last week when we were working on our forms and controllers package. Looking forward to getting this in once it has tests in core -- hopefully it's not too hard to figure out how to write the test from the existing example around line 767 in the file that @glasser mentioned.

jagill added some commits Mar 11, 2013

Now accept multiple event handlers.
Also including tests.
With this, you can define two events off of, say,
'click #myButton', and both will be triggered by a
click event.
@jagill

This comment has been minimized.

Contributor

jagill commented Mar 12, 2013

Well, the good news is that I added the tests, and corrected the bug they found (it didn't handle old style events well).

The bad news is that I merged devel into the branch, which caused many spurious commits.

I'm happy to open another, cleaner, PR if you'd like!

@glasser

This comment has been minimized.

Member

glasser commented Mar 12, 2013

You don't need to open a new PR. Get your branch in order locally however works for you, then just do a git push -f. The PR will update to be whatever you have on your branch in github.

@jagill

This comment has been minimized.

Contributor

jagill commented Mar 12, 2013

Ok, here you go!

n1mmy added a commit that referenced this pull request Mar 12, 2013

@n1mmy n1mmy merged commit bf0b8f9 into meteor:devel Mar 12, 2013

@n1mmy

This comment has been minimized.

Member

n1mmy commented Mar 12, 2013

Merged. Thanks, @jagill!

@raix

This comment has been minimized.

Contributor

raix commented Mar 15, 2013

Close issue #650 :)

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