knockout / tko

🥊 Technical Knockout – The Monorepo for Knockout.js (4.0+)
http://www.tko.io
Other
274 stars 34 forks source link

Add preventDefault to event handler options #147

Closed danieldickison closed 3 years ago

danieldickison commented 3 years ago

Because lambda (arrow functions) in bindings currently only support a single expression, the old pattern of returning true from a (e.g.) click handler is a bit inconvenient:

data-bind="click: function (event) {
    setSomeValue('foo');
    return true;
}"

Adding a preventDefault key to an event handler was discussed in #32 but it doesn't appear to have been implemented. Does this still sound like a good idea?

data-bind="click: {
    handler: (event) => setSomeValue('foo'),
    preventDefault: false
}"
brianmhunt commented 3 years ago

@danieldickison I think this scratches an itch a few people will have. 😀

maskmaster commented 3 years ago

Using a helper-function like

data-bind="click: utils.preventDefault(event => setSomeValue('foo'))"

would be a more terse variant. The exact semantics from knockout could in this case be retained.

Of course, if preventDefault needs to be observable we might need to do it like

data-bind="click: event => { setSomeValue('foo')); return $data.shouldPreventDefault; }"

A third alternative, and in my opinion more semantically correct, is to create your own binding handler "capturedClick" or something like that, where you describe the functionality instead of implementing/open-coding it in the view.

tscpp commented 3 years ago

Isn't this what we have a viewModel for? This will put logic into the view which has always been avoided in knockout which has been one of the main reasons to prefer it from any other framework or library.

This will also be very confusing when reusing an even handler method. Will the method depend on the view to preventDefault?

Also, what about stopPropagation() and stopImmediatePropagation()? These functions and preventDefault() is functions, not options.

brianmhunt commented 3 years ago

I tend to think there are two "correct" solutions here.

1. The best option for a system where you know what you need and have total control is what @maskmaster suggested:

A third alternative, and in my opinion more semantically correct, is to create your own binding handler "capturedClick" or something like that, where you describe the functionality instead of implementing/open-coding it in the view.

No library changes would be needed for this.

2. a preferable option for a "drop-in" pre-built library, when experimenting (e.g. before knowing what works), or behavioural visibility is better in the binding, can be to have a parameterized system...as @tscpp noted:

Also, what about stopPropagation() and stopImmediatePropagation()? These functions and preventDefault() is functions, not options.

in which case stopPropagation and stopImmediatePropagation ought to be parameters in the library as well.

There's nothing stopping folks from using a function e.g. utils.preventDefault but that doesn't require any library changes.

brianmhunt commented 3 years ago

Closing this as I trust it's closed, but we can always re-open if there's more to discuss here.