-
Notifications
You must be signed in to change notification settings - Fork 35
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
Add preventDefault option to event bindings #148
Conversation
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.
Thanks for this, looks like there's a few minor things and this would be ok to merge.
Also add test for static value of preventDefault.
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.
Looks good to me.
Sorry the CI tests don't run automatically on commit, but I trust the tests pass as expected.
Feel free to merge.
On a release plan, I'm working on a Github action for releases. Note that it'll be with esbuild, which is not as well tested as the rollup version, so I'll be starting with 4.1.0... release tags.
applyBindings({ noop: function () {} }, testNode) | ||
triggerEvent(testNode.childNodes[0], 'click') | ||
// Assuming we haven't been redirected to http://www.example.com/, this spec has now passed | ||
}) | ||
|
||
it('Should allow default action by setting preventDefault:false', function () { | ||
testNode.innerHTML = "<div data-bind='event: {click: test}'><a href='#' data-bind='event: {click: {preventDefault: false}}'>hey</a></div>" |
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.
We could also test preventDefault: true
here, though the code path for that is identical to the unwrap()
tested below so I'm not concerned.
Am I able to merge this into the upstream myself? I assumed you'd need to initiate that merge. (Sorry, I'm not too familiar with the workflow in github.) |
I'm not sure. 😄 I merged it in any case, now. |
Here's my attempt at #147 to add a
preventDefault
option to event bindings, as an alternative to having the handler function return a boolean.