Skip to content
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

Event binding improvements #2358

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cervengoc
Copy link
Contributor

This PR contains several minor improvements related to the event and submit bindings.

  • Refactored the submit binding to reuse the event binding. This way some concepts can be easily shared, like preventing default behavior or stopping bubbling.
  • Added a check in the bound event handler to throw an exception in case of the bound value is not a function. This adds some level of additional usability and makes debugging strange issues easier.
  • Added support for stopping event propagation from the handler by returning an explicit false value. This behavior is similar to how jQuery handles return values from event handlers and it adds support for run-time event propagation handling (in contrast to the existing "design-time" *Bubble feature).

The latter point introduces somewhat a breaking change, but I don't think that many code is affected as returning false explicitly to prevent default behavior and stop propagation is an old convention in my opinion.

Please let me know if we should refine anything here.

@cervengoc
Copy link
Contributor Author

For some reasons phantomjs tests seem to fail, I'll have a deeper look at it in the days.

@cervengoc
Copy link
Contributor Author

cervengoc commented Mar 8, 2018

@mbest I've double-checked several times the code and cannot see why it doesn't work in phantomjs tests. Could you please have a look at it?

Also, for some reason I can't run the phantomjs tests locally after a clean npm install, it just doesn't seem to do anything.

@@ -21,6 +21,10 @@ ko.bindingHandlers['event'] = {
ko.utils.registerEventHandler(element, eventName, function (event) {
var handlerReturnValue;
var handlerFunction = valueAccessor()[eventName];
if (handlerFunction !== null && handlerFunction !== undefined && typeof handlerFunction !== "function") {

Choose a reason for hiding this comment

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

you could just do typeof handlerFunction !== function, the null and undefined check seems redundant in my opinion, shouldn't those be covered by the !== function check?

Copy link
Contributor Author

@cervengoc cervengoc Mar 9, 2018

Choose a reason for hiding this comment

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

No, in this case this condition is stricter and that was intentional. I wanted to allow null or undefined values as those can be kind of valid values in some cases.

Choose a reason for hiding this comment

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

Ah, good point. I was under the assumption we didn't want null or undefined here. Thanks for the explanation.

@cervengoc
Copy link
Contributor Author

@mbest Would you please share your concerns or thoughts on this one if you have any?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants