Uncheckable radio buttons

[duncanbeevers]

Add uncheckable radio buttons, a hybrid between checkboxes and radio buttons.

[pkozlowski-opensource]

@duncanbeevers this looks like a nice addition. I'm only a bit worried about the length of the new directive's name, it becomes a bit long to type. Don't have any better proposal atm, though...

[bekos]

Can't this be implemented inside the btnRadio as an option whether to allow nothing checked?

[pkozlowski-opensource]

@bekos yup, you are right, this could be another approach

[duncanbeevers]

@bekos Sure, I'm not married to this approach. It was simply very straight-forward and obvious how to implement and use things this way. Extending btn-radio with an option would lead to less code duplication, but the cost is so low, and the path to unification in the future is so obvious, I'm not sweating it.

[bekos]

@duncanbeevers I don't have a problem either with an extra directive, but my issues were that I couldn't understand the actual purpose of this directive by it's name, so I had to look at the code (probably you need to add more things at the readme) and the code duplication as you said.

Why don't you try to unify things from now, because there is already a lot of code shared between the existing directives?

[duncanbeevers]

@bekos I've pushed a unified approach, simply decorating uncheckable radio button choices with an additional btn-radio-uncheckable attribute.

There's less overall duplication although the interface for the end-user feels kind of unwieldy.

[bekos]

I am not against a new directive :-) What I would propose though is to rename the btn-radio-uncheckable to just uncheckable and create the new btn-radio-uncheckable directive that in the compile phase will just replace itself with a btn-radio with the uncheckable as attribute.

This way user will still be able to use btn-radio-uncheckable as directive but there is no code duplication.

[duncanbeevers]

@bekos I've been playing with this approach, but it feels a little wrong. Mainly, it feels odd to have two avenues to the same behavior.

<label ng-model="model" btn-radio-uncheckable="'Value'">Stuff</label>

and

<label ng-model="model" btn-radio="'Value'" uncheckable>Stuff</label>

I guess I like the second one better since we eliminate the code duplication and we clearly indicate a right way of doing things.

Dealing with a new directive whose only job is to decorate an element for another directive doesn't seem to be buying us anything but added complexity.

[chrisirhc]

Just wondering how is this going to be different from disabled radio buttons? In Bootstrap, you can disabled radio buttons with the disabled class.

[duncanbeevers]

@chrisirhc These are uncheckable, not disabled. I've mirrored my build of the documentation here so you can play with the behavior.

Note how in the original, blue set of radio buttons, clicking an already-selected button has no effect. Also note how in the new, green set of radio buttons, clicking an already-selected button unchecks the button and sets the model value to undefined.

[chrisirhc]

Perhaps attrs.uncheckable !== undefined || !element.hasClass(buttonsCtrl.activeClass) would simplify this further.

[chrisirhc]

@duncanbeevers , thanks for explaining. Looks good to me.

[duncanbeevers]

@chrisirhc I re-pushed this, changing the hasOwnProperty check to a simple !== undefined.

[bekos]

@duncanbeevers This if keeps confusing me every time I read it :-) The reason is that you check the viewValue but this is dependent on the active state of the button.

What do you think of something like:

var isActive = element.hasClass(buttonsCtrl.activeClass);
if (!isActive || angular.isDefined(attrs.uncheckable)) {
     var viewValue = !isActive ? scope.$eval(attrs.btnRadio) : null;
     ....
}

[duncanbeevers]

@bekos I see what you're saying. I'm relying on the implicit undefined from the var declaration, and the variable name isn't the best. What do you think of this?

element.bind(buttonsCtrl.toggleEvent, function () {
  var isActive = element.hasClass(buttonsCtrl.activeClass),
      shouldUpdateValue = !isActive || angular.isDefined(attrs.uncheckable);

  if (shouldUpdateValue) {
    var newValue = isActive ? undefined : scope.$eval(attrs.btnRadio);
    scope.$apply(function () {
      ngModelCtrl.$setViewValue(newValue);
      ngModelCtrl.$render();
    });
  }
});

[bekos]

@duncanbeevers I would not create another variable, ie shouldUpdateValue, just to use it once in an if statement 1 line below. If this seems a bit magic, it's better to add a comment, rather than trying to clarify things with variable names.

About the undefined, I think the tests pass even if you use null, but I am not sure :-). If yes and the undefined has a special meaning, you should provide another test for this.

[duncanbeevers]

@bekos Alright, I've consolidated the locals to just isActive and inlined the shouldUpdateValue and newValue expressions. It all still seems to read pretty easily.

I think the tests will pass with anything falsey, which is specific enough for me.
I'll leave it as-is. If someone needs to change this, I'll revisit the tests then.

I like the null, and have changed the implementation to use it instead of undefined.

[bekos]

LGTM :-)

[dvdplm]

@bekos can we see this merged soon? :)

[bekos]

@duncanbeevers The second button still has the btn-radio-uncheckable.

[duncanbeevers]

@bekos Thanks for checking this out. I've updated the specs.

[bekos]

@duncanbeevers Can you squash your commits into one so @pkozlowski-opensource can review this also? This is his field :-)

[duncanbeevers]

@bekos @pkozlowski-opensource Squashed.

[duncanbeevers]

@bekos @pkozlowski-opensource Rebased on master (890e2d3)

[duncanbeevers]

Ping @pkozlowski-opensource @bekos This feature is rebased off a fresh master, has a green build, +1s from two developers, and should be ready to merge.

[bekos]

@duncanbeevers Finally landed :-) Thx!

[duncanbeevers]

Thanks @bekos!