rniemeyer commented 12 years ago

Adding some information here related to changes in 1.3 that might be interesting to this project.

1.3 now supports the idea of a bindingProvider. The default bindingProvider reads bindings off of the "data-bind" attribute and converts them to an object in the proper context. This bindingProvider can be swapped out, which could allow this project to create an unobtrusiveBindingProvider.

Here is a sample where the bindings live on an object and do not need to be applied in any other way to the element: http://jsfiddle.net/rniemeyer/P66Df/

There are also new functions (ko.dataFor and ko.contextFor) that can be used to retrieve the information from an element about the data that could have been bound against it. This is useful for many reasons. One is to use it with event delegation, like jQuery's live/delegate to avoid attaching event handlers to every element. Here is the sample sample with the "remove" functionality added using this idea: http://jsfiddle.net/rniemeyer/x4v4Q/

1.3 should be in beta in the next week.

bsatrom commented 12 years ago

At first glance, I would say this kind of obviates the need for Knockout.Unobtrusive, doesn't it? Not that I have a problem with that. Unobtrusive binding support is great, and having it officially in the framework is even better. :)

Let me know if I'm missing something though...

-----Original Message----- From: rniemeyer [mailto:reply@reply.github.com] Sent: Thursday, August 25, 2011 4:14 PM To: Brandon Satrom Subject: [Knockout.Unobtrusive] Supporting Knockout 1.3 (#12)

Adding some information here related to changes in 1.3 that might be interesting to this project.

1.3 now supports the idea of a bindingProvider. The default bindingProvider reads bindings off of the "data-bind" attribute and converts them to an object in the proper context. This bindingProvider can be swapped out, which could allow this project to create an unobtrusiveBindingProvider.

Here is a sample where the bindings live on an object and do not need to be applied in any other way to the element: http://jsfiddle.net/rniemeyer/P66Df/

There are also new functions (ko.dataFor and ko.contextFor) that can be used to retrieve the information from an element about the data that could have been bound against it. This is useful for many reasons. One is to use it with event delegation, like jQuery's live/delegate to avoid attaching event handlers to every element. Here is the sample sample with the "remove" functionality added using this idea: http://jsfiddle.net/rniemeyer/x4v4Q/

Reply to this email directly or view it on GitHub: https://github.com/bsatrom/Knockout.Unobtrusive/issues/12

rniemeyer commented 12 years ago

The default bindingProvider will only deal with data-bind attributes. So, this plugin would still be able to work as it does now. However, the ability to swap in a new bindingProvider creates the possibility for this plugin to become a "unobtrusiveBindingProvider" where you could simplify the design and avoid the work of even populating the data-bind attribute by having the bindingProvider read the binding object directly. Just something to think about... Given the extensibility point that will exist, I think that it makes some sense.

bsatrom commented 12 years ago

Good points, thanks Ryan, and I like the idea of saving setting every attribute with a custom provider. I'll take a look at 1.3 this week.

scottmessinger commented 12 years ago

Any news on this? I'd love to test it out for you if you're looking for some beta users of the new version.

bsatrom commented 12 years ago

Scott,

Still on my list, but I haven't gotten to it yet, unfortunately. Sorry about that. I will take a look by this weekend, though, and let you know what I throw together.

bsatrom commented 12 years ago

Now that 2.0 is out, I think it's time to revisit this.

rniemeyer commented 12 years ago

Would be happy to kick around some ideas for this one. I'll see if I can take a closer look at how this might work with a custom binding provider.

bsatrom commented 12 years ago

Hey Ryan,

That would be awesome, I'd love to have the help. I created a feature branch for this yesterday, and started stubbing out some tests based on Steve's post and previous fiddles you've shared. Documentation here is sparse, so it would be cool if we can use this as an example for other folks hoping to do the same.

On 2/14/12 7:50 PM, "rniemeyer" <reply+i-1486050-d56e02899ff697f3f4360a76bce185abe8f9381a-108356@reply.gith ub.com> wrote:

Would be happy to kick around some ideas for this one. I'll see if I can take a closer look at how this might work with a custom binding provider.

Reply to this email directly or view it on GitHub: https://github.com/bsatrom/Knockout.Unobtrusive/issues/12#issuecomment-397 3694

rniemeyer commented 12 years ago

This blog post might help as well: http://www.knockmeout.net/2011/09/ko-13-preview-part-2-custom-binding.html

bsatrom commented 12 years ago

Thanks Ryan, that post did help clarify it a ton. I have a branch with the beginnings of the binder, and one passing test, so we're rolling along. Should be pretty downhill from here.

rniemeyer commented 12 years ago

Looked at your code a bit.

I know that it is just getting started, but here are some thoughts/comments (I am looking at the JavaScript output):

inspectAttributes might fail if an element has an id that is not in the bindings, but does have a name/class that is.
applyBinding needs to return the binding, it is currently returning console.log(binding)
getFor is returning an array [{ text: "blah"}, {visible: true }], rather than an object { text: "blah", visible: true } I was using KO's extend function (ko.utils.extend) for this as a single bindingAccessor could return an object with multiple bindings and we want to merge into into the results.
the test doesn't look like it is performing an actual test yet (at least in what was committed)
might want to consider calling the default nodeHasBindings and getBindings in addition to your code, so it is still possible to use data-bind attributes. Could even be optional/configurable.

Some general thoughts: -the original plugin is keyed off of the binding names and can use conventions nicely. If you want to support this API still, then you could take in that structure and build a bindings object from it. The object would be keyed by id/class/name and have a value of the appropriate binding accessor. Maybe that was what your plan was already.

Look forward to seeing where it goes from here. Let me know how I can help.

bsatrom commented 12 years ago

Hmm, I don't think that's the latest. Did you try to run the tests?

Thx for the feedback!

From: rniemeyer Sent: 2/16/2012 10:18 PM To: Brandon Satrom Subject: Re: [Knockout.Unobtrusive] Supporting Knockout 2.0 (prev 1.3) (#12)

Looked at your code a bit.

I know that it is just getting started, but here are some thoughts/comments (I am looking at the JavaScript output):

inspectAttributes might fail if an element has an id that is not in the bindings, but does have a name/class that is.
applyBinding needs to return the binding, it is currently returning console.log(binding)
getFor is returning an array [{ text: "blah"}, {visible: true }], rather than an object { text: "blah", visible: true } I was using KO's extend function (ko.utils.extend) for this as a single bindingAccessor could return an object with multiple bindings and we want to merge into into the results.
the test doesn't look like it is performing an actual test yet (at least in what was committed)
might want to consider calling the default nodeHasBindings and getBindings in addition to your code, so it is still possible to use data-bind attributes. Could even be optional/configurable.

Some general thoughts: -the original plugin is keyed off of the binding names and can use conventions nicely. If you want to support this API still, then you could take in that structure and build a bindings object from it. The object would be keyed by id/class/name and have a value of the appropriate binding accessor. Maybe that was what your plan was already.

Look forward to seeing where it goes from here. Let me know how I can help.

Reply to this email directly or view it on GitHub: https://github.com/bsatrom/Knockout.Unobtrusive/issues/12#issuecomment-4014919

rniemeyer commented 12 years ago

Just meant that it wasn't testing the binding yet:

get('#firstName').setAttribute('value', "Steve")

equals(get('#firstName').getAttribute('value'), "Steve");

bsatrom commented 12 years ago

Ah crap.

So, yeah. I totally whiffed on this one. Really funny now that I go back and look at that test. This is what I get for trying to get at least one passing test before wrapping, and doing so with the kids hanging all over me. :)

Now that I have the right test, things are good and failing again. I'm sure I'm pretty close, hopefully I can look at it this weekend. Thanks for your feedback, again Ryan. Keepin' me honest, I like it. :D

headdesk

On 2/16/12 11:45 PM, "rniemeyer" <reply+i-1486050-d56e02899ff697f3f4360a76bce185abe8f9381a-108356@reply.gith ub.com> wrote:

Just meant that it wasn't testing the binding yet:

get('#firstName').setAttribute('value', "Steve")

equals(get('#firstName').getAttribute('value'), "Steve");

Reply to this email directly or view it on GitHub: https://github.com/bsatrom/Knockout.Unobtrusive/issues/12#issuecomment-401 5577

rniemeyer commented 12 years ago

This is just a dev branch, no big deal. You are getting the right structure in place and are on the right track. Have kids hanging over me at this very moment, so I know how that is :)

bsatrom commented 12 years ago

Ryan,

Some specific answers to your feedback above:

"inspectAttributes might fail...". - Good point, and it looks like it will. will plug in a failing test
"applyBinding needs to return the binding, it is currently returning console.log(binding)" - happy commit-finger. Fixed
"getFor is returning an array [{ text: "blah"}, {visible: true }], rather than an object { text: "blah", visible: true }" - thanks, fixed
"the test doesn't look like it is performing an actual test yet (at least in what was committed)" - fixed
"might want to consider calling the default nodeHasBindings and getBindings in addition to your code" - Good point. I'll add that to the list.

"the original plugin is keyed off of the binding names and can use conventions nicely. If you want to support this API still, then you could take in that structure and build a bindings object from it. The object would be keyed by id/class/name and have a value of the appropriate binding accessor. Maybe that was what your plan was already."

That is my plan, but I am also considering shipping multiple providers with this plugin. A complaint I've seen about KO.U is that it's too focused on binding types, rather than on named properties that align more closely to a viewModel. So I thought it wouldn't hurt to provide multiple binding providers, one that mirrors the existing way (typeProvider) and one that aligns with a more model-focused approach (propertyBinder). The property binder is what I'm working on now.

With all of that, I'm actually toying with the idea of shutting down Knockout.Unobtrusive and shipping these custom bindingProviders as a new project. Tentative name at this point is Knockout.Announcer.

Anyway, let me know what you think about all of this, and what I have so far.

rniemeyer commented 12 years ago

@bsatrom - sounds good. will have to take a closer look at the changes. Moving it to a new project sounds fine to me. Announcer is catchy. Unobtrusive is kind of nice, because people get the point of it without more context.

Would be happy to help with the project, wherever it lives.

bsatrom commented 12 years ago

good points, both. Maybe its fine as is. I've never been crazy about the name, but it is clear.

bsatrom / Knockout.Unobtrusive

Supporting Knockout 2.0 (prev 1.3) #12

headdesk