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

jQuery event is not passed on when adding arguments to binding #296

Closed
juho opened this issue Oct 9, 2017 · 13 comments
Closed

jQuery event is not passed on when adding arguments to binding #296

juho opened this issue Oct 9, 2017 · 13 comments

Comments

@juho
Copy link

juho commented Oct 9, 2017

button($b="click: addTag") Test

This passes the jQuery event to addTag, but:

button($b="click: addTag(context, tag)") Test

this does not, therefore preventing use of preventDefault() (and getting the element via currentTarget etc.) on events. Would be nice if the event is the last argument passed to it automatically so it could be implemented like

addTag(context, tag, evt) {
  evt.preventDefault();
}
@arggh
Copy link

arggh commented Oct 9, 2017

This would be a welcome addition for Blaze ViewModel too.

@jamesgibson14
Copy link
Contributor

1+

@ManuelDeLeon
Copy link
Owner

I've been thinking about this (both React and Blaze) and the least worse option I can think of is to send the event as a magical last parameter to the function:

doSomething(num, event) {
  ...
},
render() {
  <button b="click: doSomething(1)">Do Something</button>
}

This is a bad position because I don't know what I dislike the most, parameters that do something based on the name of the variable used (event, evt, etc.), or stealth parameters sent to functions at the end. I'm leaning towards the latter but I still on the fence about it.

@ManuelDeLeon
Copy link
Owner

You know what, someone had to have thought of this already. Let's split the work of finding out how things are done elsewhere and pick the best one.

@juho Find out how Aurelia would deal with a situation like this.
@arggh Find out how Knockout would deal with this.
@jamesgibson14 Find out how Vue would deal with this.
I'll see how Angular does it.

Right now our best option (IMO) is to pass the event as an implicit last parameter.

@juho
Copy link
Author

juho commented Oct 10, 2017

Aurelia and Knockout seem to do some magic binding stuff. IMO passing the event as the last prop is the best approach, then you can use it if you want to and it's a simple 1 line addition to the docs.

@juho
Copy link
Author

juho commented Oct 10, 2017

FYI Manuel it would be great if the main files would be in JS at some point instead of Coffeescript, that kinda limits my contributions to code atm. Would like to take a look at perf problems or offer a bounty for one but the code's (mostly) uncommented and in CS, so it's a drawback as you're probably the only one who understands how all of it works. :)

@arggh
Copy link

arggh commented Oct 10, 2017

When calling without parameters, KnockOut behaves partly similar to ViewModel. The event is provided as the last parameter and the calling model is provided as the first parameter.

<button data-bind='click: registerClick'>Click me</button>
var ClickCounterViewModel = function() {
    this.registerClick = function(data, event) {
        console.log(data); // calling vm
        console.log(event); // MouseEvent
    };
};

If the developer wants to add parameters, using a function literal is the Knockout-way (I'm not an immediate fan):

<button data-bind='click: function(data, event) { registerClick(5, event) }'>Click me x 5</button>
var ClickCounterViewModel = function() {
    this.registerClick = function(clickAmount, event) {
        console.log(event); // MouseEvent
    };
};

When using the first method, Knockout also calls .preventDefault() on your behalf, and the developer has to return true from the handler in order to allow the default functionality. I'm not a fan of this approach, I'd rather handle canceling & bubbling the old way, myself.

Btw, I didn't quite understand what's not to like about adding the event as the last parameter? That would have been the obvious path if you asked me.

Also, I'm 100% with @juho on ViewModel being converted to modern JavaScript instead of CoffeeScript to make it easier to contribute. Just so happens I tried this two days ago with Decaffeinate and Prettier, but as expected, it wasn't that easy. Also, some comments on the code might make it easier for other people to contribute!

@jamesgibson14
Copy link
Contributor

jamesgibson14 commented Oct 10, 2017

From Vue (https://vuejs.org/v2/api/#v-on), related to v-on:

When listening to native DOM events, the method receives the native event as the only argument. If using inline statement, the statement has access to the special $event property: v-on:click="handle('ok', $event)".

So they have a special parameter that you can pass into the template string, which I think would work great, I wouldn't mind it being passed as the last argument either, just because I need it in some of my functions, mostly to stop propagation and prevent default actions.

@juho
Copy link
Author

juho commented Oct 10, 2017

Adding another prop to the template kinda goes against the less is more idea imo, just passing the event as the last prop seems like the most elegant thing and easier to start take advantage of. Currently it feels like click: foo works correctly with the event passed "magically" so click: foo(bar) should have a similar magical experience, my 2c. It's a gotcha that makes sense at least.

@ManuelDeLeon
Copy link
Owner

After this discussion it's clear that passing it as a last (implicit) parameter is the way to go. Like juho mentioned, it's the way it works right now if no parameters are given.

I was probably against it because the first impression of using a convention parameter like event or $event. Talk about anchoring.

I should be able to add quickly to both VM versions.

As for VM-Blaze switching to JS, we have to work out a way for someone (one of you?) to own the project. Right now I can only give minimal love to VM-Blaze (relatively small enhancements like this one, bug fixes, etc.) More on that on the other thread.

btw, Angular uses a named variable too $event.

@ManuelDeLeon
Copy link
Owner

manuel:[email protected] now passes the event to the methods.

@juho
Copy link
Author

juho commented Oct 22, 2017

Cool, thanks!

@jamesgibson14
Copy link
Contributor

Work great, thank you.

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

No branches or pull requests

4 participants