Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): allow triggerHandler() to accept custom event #8505

Closed
wants to merge 3 commits into from

Conversation

petebacondarwin
Copy link
Contributor

In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (eventName). If it is an object with a type property
then we assume that it must be a custom event.

In this case the custom event must provide the type property which is
the name of the event to be triggered. triggerHandler will continue to
provide dummy default functions for preventDefault(), isDefaultPrevented()
and stopPropagation() but you may override these with your own versions
in your custom object if you wish.

Closes #8469

@caitp
Copy link
Contributor

caitp commented Aug 6, 2014

This is one of those features that was left out of jqLite on purpose, but the implementation actually looks easier to read to me, so I think this is okay.

@mzgol should review this though, imo

}];
};

if ( eventName.type ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

lets not use the jquery codestyle here though :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry that is my dgeni coding style coming through :-)

@petebacondarwin
Copy link
Contributor Author

Why was it left out?

@caitp
Copy link
Contributor

caitp commented Aug 6, 2014

I wasn't here at the time it was left out, but my guess is that it was just simpler. Still, I think the code is a bit easier to read this way, so even if it's an extra feature it's still nice

eventName = event.type;
}

var handlerArgs = [event].concat(eventData || []);
Copy link
Contributor

Choose a reason for hiding this comment

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

don't create memory garbage needlessly. use ternary operator plz

@IgorMinar
Copy link
Contributor

otherwise lgtm

In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

Closes angular#8469
@petebacondarwin
Copy link
Contributor Author

I have updated the algorithm to be exit earlier if there are no handlers (!eventFns) and less wasteful of memory (dummyEvent is only created if there are handlers, and it is only extended if a custom event is provided; also we only call [dummyEvent'].concat() if there are extra parameters).

I also changed the parameter name from eventData to extraParameters because eventData is actually the name of an argument to the on() method not triggerHandler. See http://api.jquery.com/trigger/#entry-longdesc:

Note the difference between the extra parameters passed here and the eventData parameter to the .on() method. Both are mechanisms for passing information to an event handler, but the extraParameters argument to .trigger() allows information to be determined at the time the event is triggered, while the eventData argument to .on() requires the information to be already computed at the time the handler is bound.

@petebacondarwin
Copy link
Contributor Author

@IgorMinar says we can go ahead and merge this

@caitp
Copy link
Contributor

caitp commented Aug 8, 2014

it might be a good idea to wait until they're done with the jquery stuff, but it's probably fine.

@caitp
Copy link
Contributor

caitp commented Aug 8, 2014

anyways, lgtm

@petebacondarwin
Copy link
Contributor Author

Closed via 01d81cd

petebacondarwin added a commit that referenced this pull request Aug 11, 2014
In some scenarios you want to be able to specify properties on the event
that is passed to the event handler. JQuery does this by overloading the
first parameter (`eventName`). If it is an object with a `type` property
then we assume that it must be a custom event.

In this case the custom event must provide the `type` property which is
the name of the event to be triggered.  `triggerHandler` will continue to
provide dummy default functions for `preventDefault()`, `isDefaultPrevented()`
and `stopPropagation()` but you may override these with your own versions
in your custom object if you wish.

In addition the commit provides some performance and memory usage
improvements by only creating objects and doing work that is necessary.

This commit also renames the parameters inline with jQuery.

Closes #8469
Closes #8505
@petebacondarwin petebacondarwin deleted the issue-8469 branch December 17, 2015 12:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strange behavior of triggerHandler in jqLite
3 participants