mikeric / rivets

Lightweight and powerful data binding.
http://rivetsjs.com
MIT License
3.23k stars 310 forks source link

Publish to radio button group always selects the last input #433

Open kyleseely opened 9 years ago

kyleseely commented 9 years ago

http://jsfiddle.net/d7f0rm2t/

In the example above publish always selects the last radio button. Am I doing something wrong?

Duder-onomy commented 9 years ago

If I remove the view.publish(); from your example, everything works as expected.

I dont know why calling publish would to that, but here is the rivets source: https://github.com/mikeric/rivets/blob/master/src/bindings.coffee#L105

kyleseely commented 9 years ago

That's exactly the bug I'm trying to show. Publish is supposed to take the state of the view and update the model appropriately. This doesn't work for radio button groups. This jsfiddle might help explain the problem better: http://jsfiddle.net/bgmx8dkq/

Duder-onomy commented 9 years ago

Ok, right on. Unsure if this is a bug. but definitely confusing.

view.publish() calls binding.publish on each binding. In your example, there are 2 bindings. One for each input. The first binding.publish is correct, it takes the checked property of true, and sets it on the object. It is that second pass that screws stuff up. That second radio's checked property is false, and thus, false gets set on the model.

So, it seems like Rivets is working correctly in the sense that it is setting the correct property on the model. But the behavior is unexpected.

I have never been able to get view.publish or view.sync to work how I expected them too. Though, 15 SPA's with Rivets later, I have never had to actually use them. I have always found a way to get it to work without relying on those methods.

@mikeric Can you tell us when view.publish is the right choice? And maybe confirm or deny if this is a real bug?

pgib commented 9 years ago

Having the same issue. Another jsfiddle demonstrating this radio group issue using Backbone: http://jsfiddle.net/pgib/m9pgs5s7/

jeron-diovis commented 9 years ago

+1 for this issue.

It's definitely not a bug. The problem is that Rivets binders are designed to work with a single element, and radio group is a unique usecase, when multiple elements represent a single one.

Though, since a #406 is included in core, possible solution could be following:

rivets.binders.ragiogroup = 
    # ... copy everything from "checked" binder
    getValue: (el) ->
       group = document.querySelectorAll("[name=#{el.name}]")

       for radio in group when radio.checked
           return radio.value

       return "" # default

Or, taking into account that radio group is, probably, placed inside a form (do you ever need inputs outside of form, yeah?), we can get rid of using querySelector overhead by allowing browser to aggregate radio group for us:

rivets.binders.ragiogroup = 
    # ...
    getValue: (el) -> el.form.elements[el.name].value

And that's all. view.publish() will work just as expected. In my project it solved this issue completely. Would be nice to have smth like this included in core binders.