Simplified Ember.RadioButtonGroup implementation #1235

Closed
wants to merge 17 commits into
from

Projects

None yet
@ghempton
Member
ghempton commented Aug 4, 2012

This PR contains the same API as #755 but has a simper implementation. Ember.RadioButton can also be used independently of Ember.RadioButtonGroup.

Also introduces Ember.Control which is a view that uses itself as its context, necessary for the Ember.RadioButtonGroup implementation.

@travisbot

This pull request passes (merged 7bea55f2 into d8f76a7).

@travisbot

This pull request passes (merged 657da83 into d8f76a7).

@caligo-mentis
Contributor

Nice implementation! But last selected button doesn't uncheck when value becomes null.

@ghempton
Member

Could you write a failing test case?

@caligo-mentis
Contributor

Shure.

test("should uncheck previous selection when new value is null", function() {
  view = Ember.RadioButtonGroup.create({
    value: 'option1',
    name: 'testName',
    template: Ember.Handlebars.compile(
      '{{ view RadioButton value="option1" }}' +
      '{{ view RadioButton value="option2" }}'
    )
  });

  appendView();

  Ember.run(function() {
    set(view, 'value', null);
  });

  equal(get(view, 'value'), null, 'value should be set');
  equal(view.$("[value='option1']").attr('checked'), null, 'checkbox should not be checked');
  equal(view.$("[value='option2']").attr('checked'), null, 'checkbox should not be checked');
});
@ghempton
Member

Thanks! Should be fixed now.

@travisbot

This pull request passes (merged a8be921 into d8f76a7).

@caligo-mentis
Contributor

Ok, much better. But now it's stop working in the browser at all :) I've send PR to your repo.

@caligo-mentis
Contributor

I also realized that there is problem with 'isChecked:checked' attribute binding. At least on Chrome. I wrote the working version today, but using custom observers on isChecked property.

@travisbot

This pull request passes (merged 9f9f58f into d8f76a7).

@ghempton
Member

@caligo-mentis The test you added seems to be passing :/. I think what this really needs is a fiddle. I might have time to put one together later today.

@caligo-mentis
Contributor

What browser you are using?
On 13.08.2012, at 20:59, Gordon L. Hempton wrote:

@caligo-mentis The test you added seems to be passing :/. I think what this really needs is a fiddle. I might have time to put one together later today.


Reply to this email directly or view it on GitHub.

@ghempton
Member

I am using chrome beta. It also passed in travis (although not sure if that is working correctly)

@caligo-mentis
Contributor

Please check this fiddle http://jsfiddle.net/ZPzU2/

The problem is what checked attribute binding does not affect on DOM element. After few changes with mouse it is not possible to update selection through bindings.

@ghempton
Member

I see. I will take a pass on this today.

On Tue, Aug 14, 2012 at 12:44 PM, Andrey notifications@github.com wrote:

Please check this fiddle http://jsfiddle.net/ZPzU2/

The problem is what checked attribute binding does not affect on DOM
element. After few changes with mouse it is not possible to update
selection through bindings.


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-7737794.

Gordon L. Hempton
http://codebrief.com
360.460.8098

@caligo-mentis
Contributor

Ok. And check my implementation http://jsfiddle.net/66QJm/

@ghempton
Member

If your implementation passes all the tests and fixes the issue above, I say we just go with that :). Want to PR my repo?

@caligo-mentis
Contributor

The issue is fixed, but I think the problem seems to be in attributeBindings.

I can write more tests for RadioButtonGroup, and create PR, but if problem is in ember.js we should fix it before.

On 14.08.2012, at 23:59, Gordon L. Hempton wrote:

If your implementation passes all the tests and fixes the issue above, I say we just go with that :). Want to PR my repo?


Reply to this email directly or view it on GitHub.

@wagenet
Member
wagenet commented Oct 8, 2012

@ghempton What's the status of this?

@ahawkins
Contributor

@ghempton can you add an example of a static radio group? I mean an example where you want to display N options to the user but they don't change.

EDIT: I just noticed this. I have my RadioButtonGroup bound to a controller:

{{#view Inbox.FilterRadioButtonGroup valueBinding="selectedFilter"}}

Should setting selectedFilter change the selected radio button? If so, this does not work ATM.

EDIT2: seems it's the same thing that @caligo-mentis mentioned. The checked property is not removed properly and the UI cannot be updated via bindings.

@ahawkins
Contributor

@wagenet Status update: I've sent a PR to @ghempton repo to fix bugs. You can find it here: ghempton#2. If he does not respond in a timely fashion then I will take over this PR.

@ahawkins
Contributor

@wagenet @tomdale @wycats @kselden If there is interest in this then I can take charge and do what it takes to get this in. Otherwise close this PR if we don't want to support radio buttons in the framework.

@ghempton
Member

Hey guys, sorry about not noticing this. My inbox has been swamped in github notifications. I will take a look at this today.

@ghempton
Member

@twinturbo I have merged in your PR and rebased against master. Are there any other outstanding issues with this PR? @caligo-mentis?

@caligo-mentis
Contributor

@ghempton everything should be ok now.

@wagenet
Member
wagenet commented Nov 25, 2012

@ghempton Any idea why it doesn't merge cleanly?

@ghempton
Member

@wagenet not sure... it was failing before due to a deprecated jQuery feature but I fixed the tests. Looks like Travis hasn't re-run them. Any ideas?

@wagenet
Member
wagenet commented Nov 26, 2012

@ghempton It's not a test failure, it's that it can't merge automatically which means there's a merge conflict.

@ghempton
Member

@wagenet gotcha. Just rebased again, should merge cleanly now.

@ahawkins
Contributor

@ghempton you should update the CHANGELOG

@ghempton
Member
ghempton commented Dec 9, 2012

@twinturbo I updated the docs (added a static example per your request) as well as added an entry in the changelog.

@ahawkins
Contributor

@wagenet can you take a look at this? It seems it's complete.

@wagenet wagenet commented on the diff Dec 14, 2012
packages/ember-handlebars/lib/controls/control.js
+/**
+ @class
+
+ A control is an Ember.View subclass which behaves slightly differently.
+
+ Specifically, a control uses itself as its context and is generally controller-less.
+
+ @extends Ember.View
+*/
+Ember.Control = Ember.View.extend({
+
+ init: function() {
+ this._super();
+
+ // Once #1234 is merged in this can be moved out of init
+ set(this, '_context', this);
@wagenet
wagenet Dec 14, 2012 Member

#1234 has been merged.

@wagenet wagenet commented on the diff Dec 14, 2012
packages/ember-handlebars/lib/controls/control.js
+require("ember-views/views/view");
+
+var get = Ember.get, getPath = Ember.getPath, set = Ember.set;
+
+
+/**
+ @class
+
+ A control is an Ember.View subclass which behaves slightly differently.
+
+ Specifically, a control uses itself as its context and is generally controller-less.
+
+ @extends Ember.View
+*/
+Ember.Control = Ember.View.extend({
+
@wagenet
wagenet Dec 14, 2012 Member

It seems odd to only subclass Ember.Control in one place. Were you intending for other controls to subclass this as well?

@wagenet
Member
wagenet commented Dec 19, 2012

@ghempton, are you able to address my two concerns?

@ghempton
Member

@wagenet I was intending for Ember.Control to be more widely used. In general we need a widget api.

@chadhietala
Member

Since this isn't merged in I attempted to extract all of the RadioButton scripts from the branch and use it in my project. One problem that I did notice is that when using the RadioGroup the last item in the group is always set to the checked state. Can anyone else replicate this? I'll update if I find a solution

@chadhietala
Member

Never mind I found a typo I made in translating it radio_buttons.js into coffeescript.

@calumbrodie

Has this one stalled? I'm working around this by using selects instead of radio buttons. One option might be to use a 'choice' object type which can have options set in order to define how its rendered as radio/select/checkboxes are quite similar in nature:

e.g

http://symfony.com/doc/master/reference/forms/types/choice.html

@owilliams owilliams commented on the diff Mar 26, 2013
packages/ember-handlebars/lib/controls/radio_button.js
+ isDisabled: false,
+
+ /**
+ Sets the checked property on the element.
+
+ @default false
+ @type Boolean
+ */
+ checked: false,
+
+ tagName: "input",
+ type: "radio",
+
+ selectedValueChanged: Ember.observer(function() {
+ var selectedValue = get(this, "selectedValue");
+ if(!Ember.empty(selectedValue) && get(this, "value") === selectedValue) {
@owilliams
owilliams Mar 26, 2013 Contributor

This would need to be updated to use Ember.isEmpty() instead of Ember.empty() for RC1

@goshakkk
Contributor

Any update on this one? I'd really like to see it merged.

@workmanw
Contributor

As would I. We're already using it in our code base, it'd be nice to have it in master.

@ahawkins
Contributor

We are using it as well. I think many people are.

On Mon, Apr 29, 2013 at 4:59 PM, Wesley Workman notifications@github.comwrote:

As would I. We're already using it in our code base, it'd be nice to have
it in master.


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-17171696
.

@ZogStriP ZogStriP added a commit to ZogStriP/discourse that referenced this pull request May 27, 2013
@ZogStriP ZogStriP FIX: You can't change a subject whilst Flagging
Meta: [You can't change a subject whilst Flagging](http://meta.discourse.org/t/you-can-t-change-a-subject-whilst-flagging/6887)

This revert a change made by @eviltrout in b794830 in the `flag_view.js`.
The `Em.run.schedule('afterRender', function() {...})` is apparently not enough. Had to schedule the event on the next run loop.

Hopefully, the PR emberjs/ember.js#1235 will soon be merged in and we'll have a proper radio button control in Ember.js.
f184090
@ZogStriP ZogStriP referenced this pull request in discourse/discourse May 27, 2013
Merged

FIX: You can't change a subject whilst Flagging #907

@npalrecha

+1

@stefanpenner
Member

thoughts on this becoming ember/ember-radio-controls addon?

@ahawkins
Contributor
ahawkins commented Jun 6, 2013

+1

On Thu, Jun 6, 2013 at 5:14 AM, Stefan Penner notifications@github.comwrote:

thoughts on this becoming ember/ember-radio-controls addon?


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-19022664
.

@workmanw
Contributor
workmanw commented Jun 6, 2013

I would personally like to see it in Ember Core. Ember.Select is in the core and it's pretty similar in principle. I'd be interested to hear what makes Ember.RadioButtonGroup different?

That said, ember/ember-radio-controls is better than this staying open for another 6 months.

@tchak
Member
tchak commented Jun 6, 2013

@stefanpenner at some point we need to have Ember.Radio in code or ditch all of the controles. I don't think ditching controles is a good idea. So we need to add radio. People a laughing at us because of radio not being supported out of the box :/

@ahawkins
Contributor
ahawkins commented Jun 6, 2013

@tchalk confirm. I laughed when I found out Radio wasn't there. What's so
bad about having radios in 1.0?

On Thu, Jun 6, 2013 at 2:17 PM, Paul Chavard notifications@github.comwrote:

@stefanpenner https://github.com/stefanpenner at some point we need to
have Ember.Radio in code or ditch all of the controles. I don't think
ditching controles is a good idea. So we need to add radio. People a
laughing at us because of radio not being supported out of the box :/


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-19041181
.

@tchak
Member
tchak commented Jun 6, 2013

@ahawkins the problem is radio button semantics are quite complex. We need to expose the right primitives.

@tchak
Member
tchak commented Jun 6, 2013

Ember.Control is reserved for something else. It is supposed to be the counterpart of Ember.Route but without url.

Ember.Radio should ba a primitive not something that tries to solve all the use cases IMHO.

@ahawkins
Contributor
ahawkins commented Jun 6, 2013

@tchalk, how are they more complex than select?

On Thu, Jun 6, 2013 at 2:26 PM, Paul Chavard notifications@github.comwrote:

@ahawkins https://github.com/ahawkins the problem is radio button
semantics are quite complex. We need to expose the right primitives.


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-19041564
.

@stefanpenner
Member

@ahawkins select is a minefield. Our primitive controls need some love.

@tchak
Member
tchak commented Jun 6, 2013

@ahawkins they group... Speaking of wich, we do not support grouping of options in selects.

@ahawkins
Contributor
ahawkins commented Jun 6, 2013

tl;dr ember form elements are embarrassing.

On Thu, Jun 6, 2013 at 2:35 PM, Paul Chavard notifications@github.comwrote:

@ahawkins https://github.com/ahawkins they group... Speaking of wich,
we do not support grouping of options in selects.


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/1235#issuecomment-19041995
.

@stefanpenner
Member

@tchak @ahawkins confirmed. Lets

@myabc
myabc commented Jun 8, 2013

+1 for releasing this as an ember/ember-radio-controls add-on

@stefanpenner
Member

I think this is for the best, providing an addon with readme + tests + travis is more accessible. It would allow the project to ge the attention it needs, and reach maturity quicker.

@jagthedrummer

+1 making this available somehow, somewhere. It's just silly that there's no way to create a radio button in Ember. It really should be a part of the core lib.

@wagenet
Member
wagenet commented Jul 19, 2013

It looks like the vote was for an add-on. If someone releases a solid add-on we can consider whether or not it's appropriate to merge into core later.

@wagenet wagenet closed this Jul 19, 2013
@guilhermeaiolfi

It should be in core, no matter what the API is. We can break compat later in future versions. But we need to support that out of the box. Embarrassing...

@stefanpenner
Member

@guilhermeaiolfi are you volunteering? The providing an alternative implementation using new Ember.Component would be useful.

@wagenet
Member
wagenet commented Aug 2, 2013

We actually can't just break the API. We're attempting to keep things stable and once we hit 1.0 (which will be soon), this becomes even more essential.

@guilhermeaiolfi

Using Ember.Component will make it much more easy to change it's internals later. We just need to define the attrs for it.

@hlubek
hlubek commented Aug 2, 2013

I'm using the new views from the PR without any Ember.Control and it just works (the change that was needed is merged like @wagenet noted). Would be great to make this available (at least as an add-on).

@hlubek hlubek commented on the diff Aug 2, 2013
packages/ember-handlebars/lib/controls/radio_button.js
+ set(this, 'selectedValue', get(this, 'value'));
+ }
+
+});
+
+/**
+ @class
+
+ The `Ember.RadioButtonGroup` view class provides a simplfied method for dealing
+ with multiple `Ember.RadioButton` instances.
+
+ ## Simple Example
+
+ ```handlebars
+ {{#view Ember.RadioButtonGroup name="role" valueBinding="content.role"}}
+ {{view RadioButton value="admin"}}
@hlubek
hlubek Aug 2, 2013

The nested view has to be accessed with {{view view.RadioButton value="..."}} in the latest RC

@denisnazarov
Contributor

Any updates here?

@askmatey
askmatey commented Sep 5, 2013

Hello ??? Any updates here ??

@alvincrespo

👍 I would totally work on this, but I need to learn how to edit pull request(s).

@kumavis
Contributor
kumavis commented Oct 31, 2013

+1

@kumavis
Contributor
kumavis commented Oct 31, 2013

@alvincrespo the correct way would be to just create a new PR

@wagenet
Member
wagenet commented Nov 1, 2013

If you submitted the PR, you can edit, but not if someone else did.

@mansona
mansona commented Nov 1, 2013

@wagenet @alvincrespo I know you can checkout the pull request and then you could effectively "fork" the pull request and make a new one. Once you do then if you mention the new pull request here it will become "linked"
(hope I helped ;)

@chainlink

+1 for this feature.

@namlook
namlook commented Jan 30, 2014

Any update about this feature ?

@cmkornell

I pulled this code from @ghempton and updated it to extend from component, fixed up the documentation, updated the tests, and added helpers. It works great aside from the one failing test that I can't seem to hammer out; although, I see it working when I build Ember and use it in production, so I think I'm just missing something really simple.

Can someone please help me pass the last test so I can re-submit this PR? This is my first attempt at working with the Ember source and the Ember Testing Suite.

I really believe this should be in the core and @ghempton did an excellent job with it; I would love to help out in any way I can to make it happen.

Here's my branch: https://github.com/cmkornell/ember.js/tree/radio-button-group
The failing test: Ember.RadioButtonGroup: value should update correctly after change event

@mansona
mansona commented Sep 4, 2014

I posted this on #4352 but i wanted to get some feedback on it and continue the discussion: https://www.npmjs.org/package/ember-radio-buttons Its a slightly simpler implementation than @ghempton 's but I think it should do for most cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment