feat(select): Adding check boxes to the multi-select menu #7455

DerekLouie · 2016-03-08T01:47:44Z

@ErinCoughlan @jelbourn @KarenParker @gmoothart

Please Review.

Adding check boxes to the multi-select menu.

References issue #3244.

devversion · 2016-03-08T15:09:58Z

src/components/select/select-theme.scss

@@ -75,3 +75,17 @@ md-select-menu.md-THEME_NAME-theme {
    }
  }
 }
+
+md-select-menu[multiple] {
+    md-option {


To be consistent, please indent with two spaces, instead of one tab

Yes- the indent should be +2 spaces

ahh apologies, my vimrc did not recognize scss and defaulted to 4 spaces. Corrected.

jelbourn · 2016-03-08T16:39:30Z

The checkbox borders in the screenshot seem exceptionally dark (black?) to me.

DerekLouie · 2016-03-08T22:38:06Z

@ErinCoughlan @jelbourn @KarenParker @gmoothart

Please Review.

I have refactored the select menu checkbox CSS to match the md-checkbox css (wherever applicable).

References issue #3244.

DerekLouie · 2016-03-09T01:24:44Z

Going to explore refactoring the checkbox css such that you can use the same classes directly.

Question @jelbourn: Where would prefer I put this css that will be used in both the select, checkbox, and potentially other components?

jelbourn · 2016-03-09T02:31:44Z

@DerekLouie there should be one single place where the styles for a checkbox lives, and then each other component should consume those styles.

DerekLouie · 2016-03-11T02:42:26Z

Hello All,

I have done some scss wizardry to make the applicable md-checkbox styles re-usable in the md-select directive (and beyond?).

Please Review.

Screenshot with RTL look and feel included for completeness.

KarenParker · 2016-03-11T18:54:22Z

LGTM

ErinCoughlan · 2016-03-11T18:56:44Z

LGTM

jelbourn · 2016-03-11T21:02:10Z

src/components/checkbox/checkbox.scss

-    }
-  }
-
+@mixin checkbox-container(


Add comment explaining what this mixin does

ThomasBurleson · 2016-03-11T21:16:26Z

@DerekLouie - we definitely need unit test(s) with this PR.

DerekLouie · 2016-03-12T02:13:28Z

Hello All,

I have added unit tests. I ran into an issue where element.find('._md-container') returned an empty sqlite object, my wild shot in the dark is that the underscore in the class name messes something up?

I ended up for looping and using some good old native JS to verify the md-checkbox markup is correctly added.

Please Review.

Best,

Derek

jelbourn · 2016-03-12T02:23:12Z

@DerekLouie angular.element's .find function only works for element names (which is surprising if you're used to using jQuery). querySelector is the way to go.

jelbourn · 2016-03-12T02:28:10Z

src/components/select/select.js

+    if (selectCtrl.isMultiple) {
+      element.attr('md-checkbox-enabled', '');
+      if (!checkboxElement) {
+        checkboxElement = angular.element('<div class="_md-container"><div class="_md-icon"></div></div>');


This isn't there yet; you're still performing the html -> dom operation for every single item, everywhere. The goal is to only do that operation once ever, and then clone the result for each item.

You want to do something like

var CHECKBOX_SELECTION_INDICATOR = angular.element('<div class="_md-container"><div class="_md-icon"></div></div>');

right inside of OptionDirective before the return, and then clone that.

I believe the behaviour you are describing is, in fact, what is going on here.

Actually if I clone element inside of the OptionDirective right before the return statement a new clone will be made every time the md-option directive is invoked (if you put a debugger on line 760 and navigate to the select demo page, you will notice you hit that debugger breakpoint several times).

I have declared the variable at the very top of the file so there should only be one reference present in the code (kept alive in the parent closure), and it is instantiated IFF we have a multi-select (as close to lazy loading as I could manage).

We never clone the element again (if you put a debugger on line 782 and navigate to the select demo page, you will notice you hit that debugger breakpoint only once).

I agree though that I should rename the variable because my intention was to create a constant.

I believe putting that constant at the top of the OptionDirective initially un-initialized but keeping the if statement would make the code more efficient than it was previously, but then detecting if the clone has been created already in other OptionDirective's on the page becomes more difficult..

Ah, I just misread the code. I wouldn't worry too much about the lazy initializing here; the cost of creating the element once at start-up should be pretty small.

DerekLouie · 2016-03-15T17:53:40Z

Hello All,

I have fixed all the aforementioned comments!

Please Review.

Best,

Derek

jelbourn · 2016-03-15T21:47:41Z

LGTM

Bouazizi-Ezzeddine · 2016-03-25T10:13:10Z

Hi,
In which version these changes are integrated ?

Closes #7455

torontom · 2016-12-05T21:11:21Z

Can I do it with anguler-material 1.0.0? I only want to make minimum changes to our project, and get the multiple select with check box...

ThomasBurleson · 2016-12-06T11:55:21Z

This feature is available in v1.1.0 or higher. See the Select Demo Pizza Toppings which uses the multiple attribute: <md-select ng-model="selectedToppings" multiple>

Version 1.0 is almost a year old and many bugs have been fixed since the v1.0.0 release.

Splaktar · 2019-07-05T22:11:42Z

This works for our standard multiple API, but it doesn't work for the undocumented, untested ng-multiple API which was introduced in 0.9.0.

I've opened #11759 to track supporting checkboxes when using ng-multiple="true"

DerekLouie changed the title ~~Adding checkboxes to angular material stuff~~ feat(select): Adding check boxes to the multi-select menu Mar 8, 2016

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

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

DerekLouie self-assigned this Mar 8, 2016

devversion reviewed Mar 8, 2016
View reviewed changes

DerekLouie force-pushed the selectPanelCheckbox branch 2 times, most recently from 4379748 to 7e29a30 Compare March 8, 2016 22:35

DerekLouie force-pushed the selectPanelCheckbox branch 2 times, most recently from 703f1db to d23334d Compare March 9, 2016 01:18

DerekLouie force-pushed the selectPanelCheckbox branch from d23334d to e37301b Compare March 11, 2016 02:40

DerekLouie force-pushed the selectPanelCheckbox branch 2 times, most recently from 0638d8a to 906a1db Compare March 11, 2016 18:31

DerekLouie force-pushed the selectPanelCheckbox branch from 906a1db to b703dc6 Compare March 11, 2016 18:59

jelbourn reviewed Mar 11, 2016
View reviewed changes

src/components/checkbox/checkbox.scss

}

}

@mixin checkbox-container(

Copy link

Member

jelbourn Mar 11, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add comment explaining what this mixin does

ThomasBurleson added needs: tests and removed needs: review This PR is waiting on review from the team labels Mar 11, 2016

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Mar 11, 2016

DerekLouie force-pushed the selectPanelCheckbox branch from b703dc6 to fc21858 Compare March 12, 2016 02:05

jelbourn reviewed Mar 12, 2016
View reviewed changes

DerekLouie force-pushed the selectPanelCheckbox branch from fc21858 to 3ce1635 Compare March 15, 2016 17:50

DerekLouie force-pushed the selectPanelCheckbox branch from 3ce1635 to e40bff0 Compare March 15, 2016 18:52

Adding checkboxes to angular material stuff

5718b52

DerekLouie force-pushed the selectPanelCheckbox branch from e40bff0 to 5718b52 Compare March 15, 2016 18:58

jelbourn added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: tests labels Mar 15, 2016

ThomasBurleson closed this in 3524aa9 Mar 16, 2016

ErinCoughlan mentioned this pull request Mar 25, 2016

feat(select): changed md-select to have select all feature #7674

Closed

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

Adding checkboxes to angular material stuff

6775d89

Closes #7455

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

Adding checkboxes to angular material stuff

c9542c3

Closes #7455

angular locked and limited conversation to collaborators Dec 6, 2016

Splaktar modified the milestones: EOC - Q1, 1.1.0 Jul 5, 2019

Splaktar added the type: enhancement label Jul 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(select): Adding check boxes to the multi-select menu #7455

feat(select): Adding check boxes to the multi-select menu #7455

DerekLouie commented Mar 8, 2016

devversion Mar 8, 2016

jelbourn Mar 8, 2016

DerekLouie Mar 9, 2016

jelbourn commented Mar 8, 2016

DerekLouie commented Mar 8, 2016

DerekLouie commented Mar 9, 2016

jelbourn commented Mar 9, 2016

DerekLouie commented Mar 11, 2016

KarenParker commented Mar 11, 2016

ErinCoughlan commented Mar 11, 2016

jelbourn Mar 11, 2016

ThomasBurleson commented Mar 11, 2016

DerekLouie commented Mar 12, 2016

jelbourn commented Mar 12, 2016

jelbourn Mar 12, 2016

DerekLouie Mar 13, 2016

jelbourn Mar 13, 2016

DerekLouie commented Mar 15, 2016

jelbourn commented Mar 15, 2016

Bouazizi-Ezzeddine commented Mar 25, 2016

torontom commented Dec 5, 2016

ThomasBurleson commented Dec 6, 2016

Splaktar commented Jul 5, 2019

feat(select): Adding check boxes to the multi-select menu #7455

feat(select): Adding check boxes to the multi-select menu #7455

Conversation

DerekLouie commented Mar 8, 2016

devversion Mar 8, 2016

Choose a reason for hiding this comment

jelbourn Mar 8, 2016

Choose a reason for hiding this comment

DerekLouie Mar 9, 2016

Choose a reason for hiding this comment

jelbourn commented Mar 8, 2016

DerekLouie commented Mar 8, 2016

DerekLouie commented Mar 9, 2016

jelbourn commented Mar 9, 2016

DerekLouie commented Mar 11, 2016

KarenParker commented Mar 11, 2016

ErinCoughlan commented Mar 11, 2016

jelbourn Mar 11, 2016

Choose a reason for hiding this comment

ThomasBurleson commented Mar 11, 2016

DerekLouie commented Mar 12, 2016

jelbourn commented Mar 12, 2016

jelbourn Mar 12, 2016

Choose a reason for hiding this comment

DerekLouie Mar 13, 2016

Choose a reason for hiding this comment

jelbourn Mar 13, 2016

Choose a reason for hiding this comment

DerekLouie commented Mar 15, 2016

jelbourn commented Mar 15, 2016

Bouazizi-Ezzeddine commented Mar 25, 2016

torontom commented Dec 5, 2016

ThomasBurleson commented Dec 6, 2016

Splaktar commented Jul 5, 2019