Radio Button groups as views.

[jneen]

This is an extension of #519. It uses most of the same code, but allows the creation of radio buttons in pure handlebars, much like Ember.Checkbox.

{{#view Ember.RadioButtonGroup name="occupation" selectedValueBinding="..."}}
  <label>
    {{view RadioButton value="programmer"}}
    Programmer
  </label>
  <label>
    {{view RadioButton value="firefighter"}}
    Firefighter
  </label>
  <!-- etc... -->
{{/view}}

Enjoy :)

[jneen]

Oh, also, with the new {{#view}} scoping, you'll have to change {{view RadioButton ...}} to {{view view.RadioButton ...}}. That's a dynamically created view class that automatically sets up the group binding.

[travisbot]

This pull request fails (merged 2a27dea into a7017ad).

[travisbot]

This pull request fails (merged 7f4f676 into a7017ad).

[travisbot]

This pull request passes (merged d91198b into a7017ad).

[endash]

+1

[guilhermeaiolfi]

What if RadioButtonGroup could be used like a CollectionView like so:

{{view Ember.RadioButtonGroup contentBinding="App.question.answers"}}
    <label>
    {{view RadioButton valueBinding="this.value"}}
    {{answer}}
  </label>
{{/view}}

Or something like that. It seems that a lot of "code" would be repeated for every RadioButton and RadioButtonGroup could handle that better.

What do you think?

[endash]

I like the nested each/collection helper approach, then you could manually spell out the options (for static ui), or use a nested CV (for dynamic content), without overly complicating the implementation.

[guilhermeaiolfi]

I take my suggestion back. I haven't realized that I could use it like a CV and have the content elsewhere. Sorry. +1 for this.

[jneen]

Yep, for a typical radio button setup you should only have to bind to the selectedValue of the parent group.

My one reservation about this is the verbose naming. Perhaps just call it "value"?

[endash]

I'd suggest "selection" as the property.

[jneen]

+1. Also, I used the name RadioButtonGroup, but my initial idea used RadioButtons/RadioButton for the view names, but maybe that's overdoing it a bit.

[eviltrout]

+1

[ghempton]

+1 on the API, however the implementation needs some work. This PR requires that the RadioButtonGroup already be in the DOM for the selectedValue property to function correctly.

Below is a failing test case which should work:

test("selectedValue works even if the view is not in the DOM", function() {
  group = Ember.RadioButtonGroup.create({
    name: 'testName',
    template: Ember.Handlebars.compile(
      '{{ view RadioButton value="option1" }}' +
      '{{ view RadioButton value="option2" }}'
    )
  });

  set(group, 'selectedValue', 'option1');

  append(group);

  equal(get(group, 'selectedValue'), 'option1', 'selectedValue should be set');

  equal(group.$("[value='option1']").attr('checked'), 'checked', 'checkbox should be checked');
});

[travisbot]

This pull request fails (merged fae0ae7 into a7017ad).

[jneen]

hm, that's interesting. It looks like because the template hasn't rendered, the child views haven't initialized and added themselves to the group's _radioButtons list. I wonder if we could just cache selectedValue somewhere, and make sure the DOM gets it in a didInsertElement hook.

@ghempton - I'll be going out of town for a little while, so I won't get to look at this for about a week and a half. Do you feel like taking a crack at it?

[wagenet]

A couple other things to note before this can be merged.

1. ember_assert is now Ember.assert. Same goes for all ember_* methods.
2. All of the debug methods need to be on a single line. I know this can make for very long lines, but until we get better stripping code this is something we need to stick to.

[endash]

[ACK: never mind, it was throwing a separate error which was somehow getting swallowed and 'passing'] I got the failing test to pass entirely on accident, when I was refactoring the groupWill/groupDidChange into a computed setter. Trying to ascertain whether this has merely covered up an underlying problem....

[wagenet]

@endash Any updates on this?

[workmanw]

This example here seems to provide a problem for me. The 'selectedValue' binding flushes while the view is in the 'preRender' state. This causes the code to assert because the view is still in the 'preRender' state and the button views have not yet been constructed.

Assert location: https://github.com/jayferd/ember.js/blob/fae0ae76d4d2e438d0936edba0ec4a0a1c96402b/packages/ember-handlebars/lib/controls/radio_button.js#L248

{{#view Ember.RadioButtonGroup name="occupation" selectedValueBinding="..."}}
  <label>
    {{view RadioButton value="programmer"}}
    Programmer
  </label>
  <label>
    {{view RadioButton value="firefighter"}}
    Firefighter
  </label>
  <!-- etc... -->
{{/view}}

Edit: If that's not clear, I can write a test for it.

[wagenet]

@workmanw I'm not too familiar with this code so a test would be great. Thanks!

[AgileMantis]

I believe the button objects are not being bound to the group.

After poking around in Firebug (so I could inspect post DOM insertion) I discovered that the RadioButtonGroup _radioButtons Array is empty. Looking further, _addRadioButton is never called. Inside each button, the group is undefined.

I also confirmed this in the test by adding equal(group._radioButtons.length,2); to the "selectedValue works even if the view is not in the DOM" test, and it failed, with a result of 0 instead of 2.

I need to dive into this to see why, but being very new to Ember, if anyone w/experience has any ideas, let me now.

[trek]

Will this be a stopped for 1.0 beta or we can this get moved to a 1.1 milestone?

[wagenet]

Seems like a nice-to-have, but shouldn't block 1.0.

[jneen]

@AgileMantis I believe the reason the buttons aren't being bound to the group is that the view has not yet been rendered, and therefore the RadioButton objects haven't been created. They add themselves to their group on initialization (see https://github.com/emberjs/ember.js/pull/755/files#L1R45).

A solution would be to cache the selected value on the group, and make sure it's reflected in the dom via a didInsertElement hook.

[AgileMantis]

@jayferd I had read that in an earlier post so I tried it. I made the following changes and the test passed:

radio_button.js (added the following)

cachedSelectedValue: null,
didInsertElement: function() {
  if(this.cachedSelectedValue) set(this,'selectedValue',this.cachedSelectedValue);
 },

radio_button_test.js (diff)

@@ -332,7 +332,7 @@ test("selectedValue works 
     )
   });

-  set(group, 'selectedValue', 'option1');
+  set(group, 'cachedSelectedValue', 'option1'

   append(group);

EDIT: Ignore below, my template was incorrect. I was using {{view Ember.RadioButton value=value}} instead of {{view view.RadioButton value=value}}, and thus I was creating new RadioButton objects instead of calling RadioButton on RadioButtonGroup.

In the browser test, I still received a the assert failure "selectedValue can only be set to the value of a ...". That's were I discovered _radioButtons was still a blank array.

There must be something wrong with my template in my browser test. I'll look further and/or post a gist. The programmatic version in the test must differ from what I'm doing.

Anyway, assuming my browser issue is me, it appears the your belief about root case is correct.

[AgileMantis]

@jaYFRED. Your suggestion worked. We can change cachedSelectedValue to defaultSelectedValue or something more semantic (or change the internal selectedValue to something else so templates can use selectedValue).

Also, the examples in the docs in radio_button.js need tweaked (e.g. {{view RadioButton valueBinding="value"}} should be {{view view.RadioButton value=value}}, when nested inside a {{#each}}) This caused my browser issue. See working gist: http://gist.github.com/3066538

There is a missing "," between label and value in the possibleAnswers json. Lastly, @wagenet comments regarding assert and debug statements.

Would you like me to address all this and send you a pull request, or would you prefer to run with this? I can also just send the core team a pull request of Beta 1.0 if there's still time.

Thanks
Brian Ledsworth

I will also test each doc'd example and possibly add tests w/more complex templates, so users don't fall into the trap I did.

Thoughts?

[AgileMantis]

@jayferd I cleaned up the caching solution and submitted a pull request to your repo. The original failing test now works with this cached solution.

I also refactored the tests as I was getting "Should not be in a run loop at end of test" failures. I modeled setup/teardown after the other tests and changed all "foo.createElement()" calls with "append(foo)", as append creates the element as part of insertion into the DOM.

[AgileMantis]

Updated RadioButtonGroup example which uses a {{#each}} block to generate buttons. Example now uses {{view view.RadioButton...}} instead of {{view RadioButton...}}, as the later fails due to the context switch caused by the {{#each}} block.

[rushidesai]

I'm running into a similar problem as @AgileMantis with this template:

  {{#view Ember.RadioButtonGroup selectedValueBinding="sex"}}
    {{view view.RadioButton value="male"}}
    {{view view.RadioButton value="female"}}
  {{/view}}

During createChildView Ember.RadioButtonGroup.selectValue calls Ember.RadioButtonGroup.buttonForValue but _radioButtons is undefined. If I remove the selectedValueBinding from the template, the example works fine and the code hits the breakpoint for the Ember.RadioButtonGroup.RadioButton computed property. But with selectedValueBinding set, it looks like buttonForValue is being called before RadioButtons have been created.

[ghempton]

I just took a stab at a simpler implementation of this. Check out #1235.

[wagenet]

@jayferd How does this compare to @ghempton's version?

[jneen]

To be honest, I haven't looked at this issue in quite a long time, and I'm on the road right now, so I'm not sure when I'll get the time to refresh my knowledge. All I did was add the dynamically-created view thing to #519, whereas @ghempton seems to have built it from the ground up a bit more simply. I'm not sure if the simpler implementation has all the binding magic that's expected, but as long as it works okay I'd go with his.

[ahawkins]

@wagenet I think you can close this in favor of #1235.

[jneen]

👍

[ahawkins]

@jayferd I've also added some commits to #1235 to cover more edge cases. You should check it out if there :)