feat(checkbox): add indeterminate checkbox support #7643

DerekLouie · 2016-03-19T00:12:14Z

@gmoothart @ErinCoughlan @jelbourn @ThomasBurleson

This adds the indeterminate state to md-checkbox.

Please Review.

References issue #1912

ThomasBurleson · 2016-03-19T00:49:35Z

@DerekLouie - please note that we have a PR and issue naming convention.

e.g. (for this PR) feat(checkbox): add indeterminate checkbox support

devversion · 2016-03-19T12:22:37Z

src/components/checkbox/checkbox.js

        if(ngModelCtrl.$viewValue) {
          element.addClass(CHECKED_CSS);
        } else {
          element.removeClass(CHECKED_CSS);
        }
      }
+
+      function setIndeterminateState() {
+        var isIndeterminate = element.attr('indeterminate') === 'true' || false;


@DerekLouie I suggest using the util function, like so

$mdUtil.parseAttributeValue(attr.indeterminate)

Ahh I didn't know that existed, that is handy! I've decided to invalidate this code and change the indeterminate state attributes a bit, but for the sake of brevity..

Interestingly enough this broke the tests b/c $mdUtil.parseAttributeBoolean will interpret an empty string as true which we want instead to be interpreted as false (in the case no value is passed in)

@DerekLouie Yes, that's by intention 😄 . Shouldn't <md-checkbox inderterminate> work?

The convention we usually follow is that if the attribute is found with no value, then it is treated as true as this follows standard HTML attributes (readonly, disabled, etc).

So,

intermediate == true

intermediate="true" == true

intermediate="false" == false

@EladBezalel @ThomasBurleson Please correct me if I'm wrong here.

@topherfangio I agree with you. That's exactly what $mdUtil.parseAttributeValue does.

But notice, attributes like readonly, disabled or required should ignore false values.
That's why you should use in that case:

$mdUtil.parseAttributeValue(attr, false)

Ahh so it does seem like parseAttributeValue does not exist. BUT parseAttributeBoolean does exist. Unfortunately because we can accept a function now as well as a string, we can't use parseAttributeBoolean :(

DerekLouie · 2016-03-21T21:34:37Z

Hello All,

I did a minor refactor.

The appearance of the indeterminate state is now class based instead of attribute selector based. This allowed me to get rid of the "indeterminate-when" attribute and simplified the api a little.

I also fixed the aforementioned comments.

Please Review.

Best,

Derek

topherfangio · 2016-03-21T22:02:00Z

src/components/checkbox/checkbox.spec.js

+
+    describe('with the indeterminate attribute', function() {
+
+      it('should set indeterminate attr to false by default', function() {


I disagree with this test; I believe it should be set to true by default to follow our existing conventions.

Hmm.. that does make a lot of sense :).

Changed, thank you.

ThomasBurleson · 2016-03-21T22:46:58Z

src/components/checkbox/checkbox.js

+ * @param {expression=} indeterminate-checked-when This determines when the
+ * indeterminate checkbox should appear checked. The 'indeterminate' state is mutually
+ * exclusive with the 'checked' and 'unchecked' states.
+ * @param {expression=} indeterminate-click This gets triggered when the


I do not understand the feature here. At first glance, this seems to interfere with ng-click. Or is this an expression that should be invoked with the indeterminant state changes?

e.g. md-change-indeterminant

What is the use case here ?

You are correct, this is not a good attribute.

Previously when I was unsure if a check box could be selected AND indeterminate I thought the click function would be useful for special toggle cases, but in retrospect this was a poor api design decision. Removing it now.

ErinCoughlan · 2016-03-23T16:57:28Z

LGTM.

Make sure to change your commit message to be:
feat(checkbox): add indeterminate checkbox support

DerekLouie · 2016-03-23T17:04:06Z

Changed the commit message, please review again at your earliest convenience!

jelbourn · 2016-03-23T17:09:39Z

src/components/checkbox/checkbox.js

          element.addClass(CHECKED_CSS);
        } else {
          element.removeClass(CHECKED_CSS);
        }
      }
+
+      function setIndeterminateState(newValue) {
+        isIndeterminate = (newValue === false) ? false : true;


isIndeterminate = newValue !== false;

ErinCoughlan · 2016-03-23T17:23:29Z

@jelbourn Should there be aria-checked somewhere in here? I didn't see it in the initial code, so is it not helpful?

DerekLouie · 2016-03-23T19:29:29Z

Took out some extra code and also added some code to dictate the 'aria-checked' attribute.

Please review.

ErinCoughlan · 2016-03-23T19:46:07Z

src/components/checkbox/checkbox.js

+      function setIndeterminateState(newValue) {
+        isIndeterminate = newValue !== false;
+        if (isIndeterminate) {
+          element.attr('aria-checked', 'mixed');


aria-checked is never set for checked or unchecked checkboxes either. If you're going to set it to "mixed", you also should set it to true/false where appropriate.

It actually is set, and accounted for in the tests. It was a little curious though that the code isn't found in this file..

I believe it's already handled by ngAria

DerekLouie · 2016-03-24T16:56:50Z

Hello All,

I have addressed all the aforementioned comments, please review at your earliest convenience!

Best,

Derek

devversion · 2016-03-24T17:06:31Z

LGTM now, aside one comment.

DerekLouie · 2016-03-24T17:15:41Z

Apologies, there have been quite a few comments, I will happily address it, which comment?

DerekLouie · 2016-03-24T17:22:44Z

Ahh thank you @devversion, fixed the comment.

jelbourn · 2016-03-24T17:58:15Z

LGTM

EladBezalel · 2016-03-24T19:10:20Z

src/core/style/mixins.scss

@@ -251,4 +251,21 @@
    cursor: default;
  }

+  &.md-indeterminate ._md-icon {


This is not related here as well, this is very specific for checkbox

@EladBezalel I could remove the &. The rule would still be valid and it would be one class less specific?

crisbeto · 2016-03-30T16:25:24Z

LGTM

ThomasBurleson · 2016-03-30T17:27:50Z

@DerekLouie - really nice feature. I LOVE this demo:

Closes angular#7643

Closes #7643

DerekLouie force-pushed the inteterminateCheckbox branch from 702f828 to f606d66 Compare March 19, 2016 00:13

DerekLouie added needs: review This PR is waiting on review from the team EOC labels Mar 19, 2016

DerekLouie added this to the EOC - Q1 milestone Mar 19, 2016

DerekLouie self-assigned this Mar 19, 2016

DerekLouie force-pushed the inteterminateCheckbox branch 3 times, most recently from cce1af7 to 9d0b1b7 Compare March 19, 2016 00:44

devversion reviewed Mar 19, 2016
View reviewed changes

DerekLouie changed the title ~~Adding the inteterminate checkbox state to md-checkbox.~~ feat(checkbox): add indeterminate checkbox support Mar 21, 2016

devversion mentioned this pull request Mar 21, 2016

mdCheckbox: support values with 0,1 #2796

Closed

DerekLouie force-pushed the inteterminateCheckbox branch 2 times, most recently from f402acc to bcc2d8c Compare March 21, 2016 21:32

DerekLouie force-pushed the inteterminateCheckbox branch from bcc2d8c to 6328e1b Compare March 21, 2016 21:44

topherfangio reviewed Mar 21, 2016
View reviewed changes

DerekLouie force-pushed the inteterminateCheckbox branch 3 times, most recently from 630c02b to 6c36b21 Compare March 21, 2016 22:29

ThomasBurleson reviewed Mar 21, 2016
View reviewed changes

DerekLouie force-pushed the inteterminateCheckbox branch from e251b35 to eee8e13 Compare March 23, 2016 17:00

jelbourn reviewed Mar 23, 2016
View reviewed changes

DerekLouie force-pushed the inteterminateCheckbox branch from eee8e13 to 8bae422 Compare March 23, 2016 19:29

ErinCoughlan reviewed Mar 23, 2016
View reviewed changes

feat(checkbox): add indeterminate checkbox support

d63137f

DerekLouie force-pushed the inteterminateCheckbox branch from 8bae422 to d63137f Compare March 24, 2016 17:21

EladBezalel reviewed Mar 24, 2016
View reviewed changes

ErinCoughlan added pr: merge ready This PR is ready for a caretaker to review and removed needs: review This PR is waiting on review from the team labels Mar 25, 2016

crisbeto assigned ThomasBurleson and unassigned DerekLouie Mar 30, 2016

ThomasBurleson closed this in 2776ad2 Mar 30, 2016

gmoothart pushed a commit to gmoothart/material that referenced this pull request Apr 5, 2016

feat(checkbox): add indeterminate checkbox support

5050ba2

Closes angular#7643

jelbourn pushed a commit that referenced this pull request Apr 28, 2016

feat(checkbox): add indeterminate checkbox support

cd987f0

Closes #7643

ThomasBurleson pushed a commit that referenced this pull request May 17, 2016

feat(checkbox): add indeterminate checkbox support

b031104

Closes #7643

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(checkbox): add indeterminate checkbox support #7643

feat(checkbox): add indeterminate checkbox support #7643

DerekLouie commented Mar 19, 2016

ThomasBurleson commented Mar 19, 2016

devversion Mar 19, 2016

DerekLouie Mar 21, 2016

devversion Mar 21, 2016

topherfangio Mar 21, 2016

devversion Mar 21, 2016

DerekLouie Mar 21, 2016

DerekLouie commented Mar 21, 2016

topherfangio Mar 21, 2016

DerekLouie Mar 21, 2016

ThomasBurleson Mar 21, 2016

DerekLouie Mar 21, 2016

ErinCoughlan commented Mar 23, 2016

DerekLouie commented Mar 23, 2016

jelbourn Mar 23, 2016

ErinCoughlan commented Mar 23, 2016

DerekLouie commented Mar 23, 2016

ErinCoughlan Mar 23, 2016

DerekLouie Mar 23, 2016

jelbourn Mar 23, 2016

DerekLouie commented Mar 24, 2016

devversion commented Mar 24, 2016

DerekLouie commented Mar 24, 2016

DerekLouie commented Mar 24, 2016

jelbourn commented Mar 24, 2016

EladBezalel Mar 24, 2016

DerekLouie Mar 30, 2016

crisbeto commented Mar 30, 2016

ThomasBurleson commented Mar 30, 2016


		describe('with the indeterminate attribute', function() {

		it('should set indeterminate attr to false by default', function() {

feat(checkbox): add indeterminate checkbox support #7643

feat(checkbox): add indeterminate checkbox support #7643

Conversation

DerekLouie commented Mar 19, 2016

ThomasBurleson commented Mar 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekLouie commented Mar 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ErinCoughlan commented Mar 23, 2016

DerekLouie commented Mar 23, 2016

Choose a reason for hiding this comment

ErinCoughlan commented Mar 23, 2016

DerekLouie commented Mar 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DerekLouie commented Mar 24, 2016

devversion commented Mar 24, 2016

DerekLouie commented Mar 24, 2016

DerekLouie commented Mar 24, 2016

jelbourn commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Mar 30, 2016

ThomasBurleson commented Mar 30, 2016