feat(selection-list): Selection-list initial version all code(without demo) #5562

sllethe · 2017-07-06T21:42:27Z

No description provided.

jelbourn · 2017-07-07T19:02:48Z

src/lib/list/list-option.html

@@ -0,0 +1,11 @@
+<div [ngClass]="(checkboxPosition == 'before')?'mat-list-item-content' : 'mat-list-item-content-reverse'" >
+  <!--<div class="mat-list-item-content">-->


Commented out code

jelbourn · 2017-07-07T19:05:15Z

src/lib/list/list.ts

@@ -175,3 +204,122 @@ export class MdListItem implements AfterContentInit {
    return this._element.nativeElement;
  }
 }
+
+@Component({


This class needs a JsDoc description

Added doc for 'md-selection-option'

jelbourn · 2017-07-07T19:07:41Z

src/lib/list/list-option.html

@@ -0,0 +1,11 @@
+<div [ngClass]="(checkboxPosition == 'before')?'mat-list-item-content' : 'mat-list-item-content-reverse'" >


Instead of using ngClass and applying one of two classes, you should be able to have the default styles always applied and then apply the "reverse" styles when the condition is met like this:

<div [class.mat-list-option-reverse]="checkboxPosition == 'after'">

Changed '[ngClass]' to '<div [class.mat-list-option-reverse]="checkboxPosition == 'after'">'

jelbourn · 2017-07-07T19:08:57Z

src/lib/list/list.ts

+
+  @Input() checkboxPosition: 'before' | 'after' = 'after';
+
+  @ViewChild('autocheckbox') pCheckbox;


I don't think you need this @ViewChild (and all associated code), since the state of the pseudo-checkbox should be set via binding in the template

jelbourn · 2017-07-07T19:10:25Z

src/lib/list/list.ts

+  constructor(private _renderer: Renderer2,
+              private _element: ElementRef,
+              @Optional() private _slist: MdSelectionList,
+              @Optional() navList: MdNavListCssMatStyler,


Why does this deal with a nav-list? MdListOption should only be used in a selection list. Getting rid of this should let you also get rid of _isNavList and _isSelectionList

(should be assumed that this is always inside of a selection list)

Deleted navList in selection-list-option

jelbourn · 2017-07-07T19:31:06Z

src/lib/list/list.ts

+    console.log('checked or not: ' + this.pCheckbox.state + ', isSelected or not: ' + this.isSelected);
+    if(this.isSelected == false) {
+      this.isSelected = true;
+      this.selectionList.checkedItems.select(this._element.nativeElement);


Rather than using the HTMLElement as the selected value, it should use either the option itself (this).

jelbourn · 2017-07-07T19:31:31Z

src/lib/list/list.ts

+})
+export class MdSelectionListCheckboxer {
+
+  checkedItems: SelectionModel<HTMLElement> = new SelectionModel<HTMLElement>(true);


checkedItems -> _selectedOptions

This property should also live directly on MdSelectionList (you shouldn't need this class at all)

Changed checkedItems to selectedOptions. And deleted MdSelectionListCheckboxer class

jelbourn · 2017-07-07T19:33:36Z

src/lib/list/list.ts

+  styleUrls: ['list.css'],
+  encapsulation: ViewEncapsulation.None
+})
+export class MdSelectionList {


In another PR after this one, we'll need to add support for the MdSelectionList being a form control, similar to the md-select

jelbourn · 2017-07-07T19:34:24Z

src/lib/list/list.ts

+  selector: 'md-selection-list, mat-selection-list',
+  host: {'class': 'mat-selection-list'}
+})
+export class MdSelectionListCssMatStyler {}


You shouldn't need this class; you can add the mat-selection-list class in MdSelectionList

Deleted 'MdSelectionListCssMatStyler' class.

jelbourn · 2017-07-07T19:34:43Z

src/lib/list/list.ts

+    '(click)': 'onchange()',
+    '(keydown)':'onKeydown($event)',
+    '[tabIndex]': 'disabled ? -1 : 0',
+    '[attr.aria-selected]': '',


Missing binding expression for aria-selected. The expression needs to resolve to a string of either 'true' or 'false' (see how existing MdOption does this).

You'll also need a binding for aria-disabled based on the disabled state

Added binding expression for 'attr.aria-selected' and 'attr.aria-disabled'

sllethe

Comments addressed. Please take another look 👍

sllethe · 2017-07-10T19:40:18Z

src/lib/list/list.ts

+  private _disableRipple: boolean = false;
+  private _isNavList: boolean = false;
+  private _isSelectionList: boolean = false;
+  isSelected: boolean = false;


Changed isSelected to selected. And added @Input(selected) for it.(Using coerceBooleanProperty)

sllethe · 2017-07-10T22:30:58Z

src/lib/list/list.ts

+
+  constructor(private _renderer: Renderer2,
+              private _element: ElementRef,
+              @Optional() private _slist: MdSelectionList,


changed _slist to selectionList and deleted MdSelectionListCheckboxer class

sllethe · 2017-07-10T22:36:05Z

src/lib/list/list.ts

+  constructor(private _renderer: Renderer2,
+              private _element: ElementRef,
+              @Optional() private _slist: MdSelectionList,
+              @Optional() navList: MdNavListCssMatStyler,


Deleted navList in selection-list-option

sllethe · 2017-07-10T22:50:00Z

src/lib/list/list.ts

+    this._lineSetter = new MdLineSetter(this._lines, this._renderer, this._element);
+  }
+
+  onchange(): void {


Changed 'onchange()' to 'toggle()' and added doc for it.

sllethe · 2017-07-10T22:53:56Z

src/lib/list/list.ts

+})
+export class MdSelectionListCheckboxer {
+
+  checkedItems: SelectionModel<HTMLElement> = new SelectionModel<HTMLElement>(true);


Changed checkedItems to selectedOptions. And deleted MdSelectionListCheckboxer class

sllethe · 2017-07-10T23:07:13Z

src/lib/list/list.ts

+  }
+
+  onchange(): void {
+    console.log('checked or not: ' + this.pCheckbox.state + ', isSelected or not: ' + this.isSelected);


removed console.log

jelbourn · 2017-07-11T22:30:23Z

src/lib/list/list.ts

+  selector: 'md-list-option',
+  host: {
+    'role': 'option',
+    'class': 'mat-list-item',


If you add mat-list-option to this as well it will be easier to write styles for the option specifically. For example, instead of .mat-selection-list .mat-list-item you could just have mat-list-option.

Added .mat-list-option class in css files

jelbourn · 2017-07-11T22:31:47Z

src/lib/list/list.scss

+    padding: 0 $mat-list-side-padding;
+    position: relative;
+    flex-direction: row-reverse;
+    -webkit-justify-content: space-around; /* Safari */


You shouldn't need the -webkit-justify-content, since Safari 9 and 10 both support all of the flex properties unprefixed.

Sure~
Deleted -webkit-justify-content in CSS file

jelbourn · 2017-07-11T22:32:02Z

src/lib/list/list.scss

@@ -43,6 +43,19 @@ $mat-dense-list-icon-size: 20px;
    position: relative;
  }

+  .mat-list-item-content-reverse {
+    display: flex;
+    //flex-direction: row;


Commented line

removed commented line

jelbourn · 2017-07-11T22:32:35Z

src/lib/list/list-option.html

+  <md-pseudo-checkbox [state]="selected ? 'checked' : 'unchecked'" #autocheckbox [disabled]="disabled"></md-pseudo-checkbox>
+  <div class="mat-list-text"><ng-content></ng-content></div>
+
+</div>


Can you configure your IDE to add newlines at the end of files?

Added new line at each file

jelbourn · 2017-07-11T22:33:40Z

src/lib/list/list.scss

@@ -60,18 +73,18 @@ $mat-dense-list-icon-size: 20px;
    height: $avatar-height;
  }

-  &.mat-2-line .mat-list-item-content {
+  &.mat-2-line .mat-list-item-content .mat-list-item-content-reverse {


You shouldn't need to add mat-list-item-content-reverse to these existing styles; the mat-list-item-content-reverse class should only have the extra styles you need to reverse the display

Deleted 'mat-list-item-content-reverse' to these existing styles;

jelbourn · 2017-07-11T22:42:53Z

src/lib/list/list.ts

+        this.selectionList.selectedOptions.deselect(this);
+      }
+    }
+  }


The toggle function by itself can just be

toggle() { this.selected = !this.selected; this.selectionList.selectedOption.toggle(this); }

The logic that a disabled option should do anything on click should be specifically tied to the click handler rather than being in the more general toggle()

👍 Great!

jelbourn · 2017-07-11T22:44:43Z

src/lib/list/list.ts

+  host: {'role': 'listbox',
+    '[attr.tabindex]': '_tabIndex',
+    'class': 'mat-selection-list',
+    // Events


No need for category comments inside host; the different syntax makes it clear enough

jelbourn · 2017-07-11T22:45:05Z

src/lib/list/list.ts

+@Component({
+  moduleId: module.id,
+  selector: 'md-selection-list, mat-selection-list',
+  host: {'role': 'listbox',


Format like

host: { 'role': 'listbox', ... }

jelbourn · 2017-07-11T22:46:14Z

src/lib/list/list.ts

+  options: QueryList<MdListOption>;
+
+  // options which are selected.
+  selectedOptions: SelectionModel<any> = new SelectionModel<any>(true);


SelectionModel<MdListOption>

jelbourn · 2017-07-11T22:48:28Z

src/lib/list/list.ts

+          // Always prevent space from scrolling the page since the list has focus
+          event.preventDefault();
+          break;
+        case LEFT_ARROW:


The selection-list shouldn't need shortcuts for left/right

Deleted shortcuts for left/right

sllethe

Comments Addressed~. Please take another look~ 👍

sllethe · 2017-07-11T23:02:30Z

src/lib/list/list-option.html

+  <md-pseudo-checkbox [state]="selected ? 'checked' : 'unchecked'" #autocheckbox [disabled]="disabled"></md-pseudo-checkbox>
+  <div class="mat-list-text"><ng-content></ng-content></div>
+
+</div>


Added new line at each file

sllethe · 2017-07-11T23:03:30Z

src/lib/list/list.scss

@@ -43,6 +43,19 @@ $mat-dense-list-icon-size: 20px;
    position: relative;
  }

+  .mat-list-item-content-reverse {
+    display: flex;
+    //flex-direction: row;


removed commented line

sllethe · 2017-07-11T23:04:53Z

src/lib/list/list.scss

+    padding: 0 $mat-list-side-padding;
+    position: relative;
+    flex-direction: row-reverse;
+    -webkit-justify-content: space-around; /* Safari */


Sure~
Deleted -webkit-justify-content in CSS file

sllethe · 2017-07-11T23:10:14Z

src/lib/list/list.scss

@@ -60,18 +73,18 @@ $mat-dense-list-icon-size: 20px;
    height: $avatar-height;
  }

-  &.mat-2-line .mat-list-item-content {
+  &.mat-2-line .mat-list-item-content .mat-list-item-content-reverse {


Deleted 'mat-list-item-content-reverse' to these existing styles;

sllethe · 2017-07-11T23:17:49Z

src/lib/list/list.scss

@@ -182,3 +195,31 @@ $mat-dense-list-icon-size: 20px;
    }
  }
 }
+
+.mat-selection-list {
+  a {


Deleted <a> element

sllethe · 2017-07-12T00:48:48Z

src/lib/list/list.ts

+        this.selectionList.selectedOptions.deselect(this);
+      }
+    }
+  }


👍 Great!

sllethe · 2017-07-12T00:49:54Z

src/lib/list/list.ts

+@Component({
+  moduleId: module.id,
+  selector: 'md-selection-list, mat-selection-list',
+  host: {'role': 'listbox',


sllethe · 2017-07-12T00:50:01Z

src/lib/list/list.ts

+  host: {'role': 'listbox',
+    '[attr.tabindex]': '_tabIndex',
+    'class': 'mat-selection-list',
+    // Events


sllethe · 2017-07-12T00:52:16Z

src/lib/list/list.ts

+  options: QueryList<MdListOption>;
+
+  // options which are selected.
+  selectedOptions: SelectionModel<any> = new SelectionModel<any>(true);


sllethe · 2017-07-12T00:55:44Z

src/lib/list/list.ts

+          // Always prevent space from scrolling the page since the list has focus
+          event.preventDefault();
+          break;
+        case LEFT_ARROW:


Deleted shortcuts for left/right

tinayuangao · 2017-07-12T21:15:21Z

package-lock.json

@@ -305,6 +305,26 @@
      "integrity": "sha1-dO6TaJbaXkU8NBzygEQ0LBqazrc=",
      "dev": true
    },
+    "@gulp-sourcemaps/identity-map": {


Revert this file?

👍 Reverted~

tinayuangao · 2017-07-12T21:18:31Z

src/lib/list/list.scss

@@ -182,3 +214,8 @@ $mat-dense-list-icon-size: 20px;
    }
  }
 }
+
+.mat-selection-list {


Deleted that empty class

tinayuangao · 2017-07-12T21:19:07Z

src/lib/list/list.ts

@@ -159,7 +501,8 @@ export class MdListItem implements AfterContentInit {

  /** Whether this list item should show a ripple effect when clicked.  */
  isRippleEnabled() {
-    return !this.disableRipple && this._isNavList && !this._list.disableRipple;
+    return !this.disableRipple && (this._isNavList)


revert this change?

reverted changes

tinayuangao · 2017-07-12T21:20:39Z

src/lib/list/list.ts

+import {CommonModule} from '@angular/common';
+import {coerceBooleanProperty, MdLine, MdLineSetter, MdPseudoCheckbox, MdSelectionModule, SelectionModel} from '../core';
+import {FocusKeyManager} from '../core/a11y/focus-key-manager';
+import {MdCheckbox} from '../checkbox';


Remove unused imports

removed unused imports

tinayuangao · 2017-07-12T21:26:27Z

src/lib/list/list.ts

+    '[attr.aria-disabled]': 'disabled.toString()',
+  },
+  templateUrl: 'list-option.html',
+  encapsulation: ViewEncapsulation.None


Can we also make this changeDetection: ChangeDetectionStrategy.OnPush?

make md-list-option 'changeDetection: ChangeDetectionStrategy.OnPush', add chengeDetector

tinayuangao · 2017-07-12T21:28:11Z

src/lib/list/list.ts

+  /** The option components contained within this selection-list. */
+  options: QueryList<MdListOption>;
+
+  // options which are selected.


nit: use /** */

tinayuangao · 2017-07-12T21:29:05Z

src/lib/list/list.ts

+
+  /**
+   * Whether the ripple effect should be disabled on the list-items or not.
+   * This flag only has an effect for `md-nav-list` components.


mat-selection-list?

Yes~ 👍 it's should be mat-selection-list

tinayuangao · 2017-07-12T21:29:53Z

src/lib/list/list.ts

+  get disableRipple() { return this._disableRipple; }
+  set disableRipple(value: boolean) { this._disableRipple = coerceBooleanProperty(value); }
+
+  // Whether the selection-list is disabled


nit: Use /**

tinayuangao · 2017-07-12T21:31:08Z

src/lib/list/list.ts

+    this._subscribeOptions(this.options);
+
+    // When the list changes, re-subscribe
+    this.options.changes.subscribe((options: QueryList<MdListOption>) => {


Also unsubscribe this in ngOnDestroy?

Added unsubscribe for 'this.options.changes.subscribe((options: QueryList)' in onDistory();

tinayuangao · 2017-07-12T21:33:21Z

src/lib/list/list.ts

+      switch (event.keyCode) {
+        case SPACE:
+          // If we are selectable, toggle the focused option
+          if (this.selectable) {


You can remove this if since you check this.selectable in _toggleSelectOnFocusedOption

👍 Removed if (this.selectable) check

sllethe

Comments Addressed~ Please take another look 👍

sllethe · 2017-07-12T21:52:09Z

src/lib/list/list.scss

@@ -182,3 +214,8 @@ $mat-dense-list-icon-size: 20px;
    }
  }
 }
+
+.mat-selection-list {


Deleted that empty class

sllethe · 2017-07-12T21:54:50Z

src/lib/list/list.ts

+import {CommonModule} from '@angular/common';
+import {coerceBooleanProperty, MdLine, MdLineSetter, MdPseudoCheckbox, MdSelectionModule, SelectionModel} from '../core';
+import {FocusKeyManager} from '../core/a11y/focus-key-manager';
+import {MdCheckbox} from '../checkbox';


removed unused imports

sllethe · 2017-07-12T22:57:53Z

src/lib/list/list.ts

+    '[attr.aria-disabled]': 'disabled.toString()',
+  },
+  templateUrl: 'list-option.html',
+  encapsulation: ViewEncapsulation.None


make md-list-option 'changeDetection: ChangeDetectionStrategy.OnPush', add chengeDetector

sllethe · 2017-07-12T22:59:09Z

src/lib/list/list.ts

+  /** The option components contained within this selection-list. */
+  options: QueryList<MdListOption>;
+
+  // options which are selected.


sllethe · 2017-07-12T23:00:11Z

src/lib/list/list.ts

+
+  /**
+   * Whether the ripple effect should be disabled on the list-items or not.
+   * This flag only has an effect for `md-nav-list` components.


Yes~ 👍 it's should be mat-selection-list

sllethe · 2017-07-12T23:00:18Z

src/lib/list/list.ts

+  get disableRipple() { return this._disableRipple; }
+  set disableRipple(value: boolean) { this._disableRipple = coerceBooleanProperty(value); }
+
+  // Whether the selection-list is disabled


sllethe · 2017-07-12T23:22:09Z

src/lib/list/list.ts

+    this._subscribeOptions(this.options);
+
+    // When the list changes, re-subscribe
+    this.options.changes.subscribe((options: QueryList<MdListOption>) => {


Added unsubscribe for 'this.options.changes.subscribe((options: QueryList)' in onDistory();

sllethe · 2017-07-12T23:22:53Z

src/lib/list/list.ts

+      switch (event.keyCode) {
+        case SPACE:
+          // If we are selectable, toggle the focused option
+          if (this.selectable) {


👍 Removed if (this.selectable) check

sllethe · 2017-07-12T23:24:02Z

src/lib/list/list.ts

@@ -159,7 +501,8 @@ export class MdListItem implements AfterContentInit {

  /** Whether this list item should show a ripple effect when clicked.  */
  isRippleEnabled() {
-    return !this.disableRipple && this._isNavList && !this._list.disableRipple;
+    return !this.disableRipple && (this._isNavList)


reverted changes

tinayuangao · 2017-07-13T17:58:38Z

src/lib/list/list.ts

+  get disabled() { return (this.selectionList && this.selectionList.disabled) || this._disabled; }
+  set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }
+
+  @Input('value')


You can use @Input() here

tinayuangao · 2017-07-13T17:58:47Z

src/lib/list/list.ts

+  get value() { return this._value; }
+  set value( val: any) { this._value = coerceBooleanProperty(val); }
+
+  @Input('selected')


tinayuangao · 2017-07-13T18:01:30Z

src/lib/list/list.ts

+  toggle(): void {
+    if(this._disabled == false) {
+      this.selected = !this.selected;
+      this._changeDetector.markForCheck();


Why do you need markForCheck here?

Moved the position of markForCheck to the end of this function

tinayuangao · 2017-07-13T18:01:57Z

src/lib/list/list.ts

+
+  _handleFocus() {
+    this._hasFocus = true;
+    this._renderer.addClass(this._element.nativeElement, 'mat-list-item-focus');


nit: make mat-list-item-focus a const?

Added const 'FOCUSED_STYLE'

jelbourn · 2017-07-14T17:38:07Z

src/lib/list/_list-theme.scss

@@ -29,6 +33,22 @@
      background: mat-color($background, 'hover');
    }
  }
+
+  .mat-selection-list .mat-list-item {


Can these styles just be moved to .mat-list-option?

moved styles in .mat-selection-list .mat-list-item to .mat-list-option

jelbourn · 2017-07-14T17:40:03Z

src/lib/list/list.scss

+    position: relative;
+    flex-direction: row-reverse;
+    justify-content: space-around;
+  }


Do position, box-sizing, and height need to be here?

Deleted position, box-sizing, and height

jelbourn · 2017-07-14T17:45:04Z

src/lib/list/list.ts

+
+  /** Whether the label should appear after or before the checkbox. Defaults to 'after' */
+
+  @Input() checkboxPosition: 'before' | 'after' = 'after';


Have you tested this in RTL?

Yes, it works.

jelbourn · 2017-07-14T17:45:16Z

src/lib/list/list.ts

+  @ContentChildren(MdLine) _lines: QueryList<MdLine>;
+
+  /** Whether the label should appear after or before the checkbox. Defaults to 'after' */
+


Omit blank line between JsDoc and the property it describes

jelbourn · 2017-07-14T17:53:03Z

src/lib/list/list.ts

+
+  @Input() checkboxPosition: 'before' | 'after' = 'after';
+
+  /** Whether the checkbox is disabled. */


"checkbox" -> "option"

jelbourn · 2017-07-14T18:07:58Z

src/lib/list/list.ts

+    // otherwise pass to our key manager
+    if (target && (target.classList.contains('mat-selection-list') ||
+      target.classList.contains('mat-list-item') ||
+      target.classList.contains('mat-list-option'))) {


Why do you need this check on the target? If this component receives a keydown, don't you always want to handle it?

Deleted check on the target.

jelbourn · 2017-07-14T18:08:43Z

src/lib/list/list.ts

+
+    let focusedIndex = this._keyManager.activeItemIndex;
+
+    if (typeof focusedIndex === 'number' && this._isValidIndex(focusedIndex)) {


Why do you need this typeof check?

Because focusedIndex = this._keyManager.activeItemIndex and the type of keyManager.activeItemIndex is 'number | null'. If I do not check typeof, it will run out an error.

Check whether focusedIndex is null?

Yeah, checking for == null is more typical

👍 Changed to check 'focusedIndex != null'

sllethe

Comments Addressed. Please take another look 👍

sllethe · 2017-07-14T22:10:24Z

src/lib/list/_list-theme.scss

@@ -29,6 +33,22 @@
      background: mat-color($background, 'hover');
    }
  }
+
+  .mat-selection-list .mat-list-item {


moved styles in .mat-selection-list .mat-list-item to .mat-list-option

sllethe · 2017-07-14T22:24:10Z

src/lib/list/list.scss

+    position: relative;
+    flex-direction: row-reverse;
+    justify-content: space-around;
+  }


Deleted position, box-sizing, and height

sllethe · 2017-07-14T22:27:29Z

src/lib/list/list.ts

+  @ContentChildren(MdLine) _lines: QueryList<MdLine>;
+
+  /** Whether the label should appear after or before the checkbox. Defaults to 'after' */
+


sllethe · 2017-07-14T22:33:26Z

src/lib/list/list.ts

+
+  /** Whether the label should appear after or before the checkbox. Defaults to 'after' */
+
+  @Input() checkboxPosition: 'before' | 'after' = 'after';


Yes, it works.

sllethe · 2017-07-14T22:35:16Z

src/lib/list/list.ts

+
+  @Input() checkboxPosition: 'before' | 'after' = 'after';
+
+  /** Whether the checkbox is disabled. */


sllethe · 2017-07-14T23:29:49Z

src/lib/list/list.ts

+    // otherwise pass to our key manager
+    if (target && (target.classList.contains('mat-selection-list') ||
+      target.classList.contains('mat-list-item') ||
+      target.classList.contains('mat-list-option'))) {


Deleted check on the target.

sllethe · 2017-07-14T23:40:32Z

src/lib/list/list.ts

+
+    let focusedIndex = this._keyManager.activeItemIndex;
+
+    if (typeof focusedIndex === 'number' && this._isValidIndex(focusedIndex)) {


Because focusedIndex = this._keyManager.activeItemIndex and the type of keyManager.activeItemIndex is 'number | null'. If I do not check typeof, it will run out an error.

jelbourn · 2017-07-17T21:27:56Z

src/lib/list/list.ts

+    '(focus)': 'focus()',
+    '(keydown)': 'keydown($event)'},
+  queries: {
+    options: new ContentChildren(MdListOption)


Why do you add queries here instead of using @ContentChildren?

Changed to use @ContentChildren

jelbourn · 2017-07-17T21:28:36Z

src/lib/list/list.ts

+
+    let focusedIndex = this._keyManager.activeItemIndex;
+
+    if (typeof focusedIndex === 'number' && this._isValidIndex(focusedIndex)) {


Yeah, checking for == null is more typical

jelbourn · 2017-07-17T21:30:06Z

src/lib/list/list.ts

+  encapsulation: ViewEncapsulation.None,
+  changeDetection: ChangeDetectionStrategy.OnPush
+})
+export class MdSelectionList implements AfterContentInit, OnDestroy {


Can md-selection-list and md-list-option go in separate files? (selection-list.ts and list-option.ts)

👍 made md-selection-list and md-list-option go in separate files? (selection-list.ts and list-option.ts)

sllethe · 2017-07-19T00:37:10Z

Comments Addressed 👍
Please take another look.

jelbourn · 2017-07-19T18:07:40Z

src/lib/list/selection-list.ts

+    '[attr.tabindex]': '_tabIndex',
+    'class': 'mat-selection-list',
+    '(focus)': 'focus()',
+    '(keydown)': 'keydown($event)'},


This needs a binding for aria-disabled to reflect its disabled state.

👍 Binded aria-disabled

jelbourn · 2017-07-19T18:08:21Z

src/lib/list/selection-list.ts

+  selector: 'md-selection-list, mat-selection-list',
+  host: {
+    'role': 'listbox',
+    '[attr.tabindex]': '_tabIndex',


The tabindex should be -1 when the selection-list is disabled

👍 Set The tabindex of list option as -1 when the selection-list is disabled

jelbourn · 2017-07-19T18:10:10Z

src/lib/list/selection-list.ts

+  /** Whether the selection-list is disabled */
+  @Input()
+  get disabled() { return this._disabled; }
+  set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }


Instead of defining the disabled getter/setter yourself, you can include it via mixin. Check out button.ts for an example of how to apply a mixin (there's some boilerplate involved but it ultimately reduces code duplication).

Applied mixin on disabled 👍

jelbourn · 2017-07-19T18:11:40Z

src/lib/list/selection-list.ts

+    // it back to the first option when the user tabs out.
+    this._tabOutSubscription = this._keyManager.tabOut.subscribe(() => {
+      this._tabIndex = -1;
+      setTimeout(() => this._tabIndex = 0);


I don't understand why you're doing this with tabindex. AFAIK, the tabindex of the selection list should always be 0 if it is enabled and -1 if it is disabled. The individual options should all have tabindex="-1"

Yes, this part is not being needed. I've deleted this part

jelbourn · 2017-07-19T18:12:35Z

src/lib/list/selection-list.ts

+  private _tabOutSubscription: Subscription;
+
+  /** Subscription to option changes from the selection-list. */
+  private _optionSubscription: Subscription;


Maybe _optionsChangeSubscription?

Renamed to '_optionsChangeSubscription'

jelbourn · 2017-07-19T18:13:38Z

src/lib/list/selection-list.ts

+  get selectable(): boolean { return this._selectable; }
+  set selectable(value: boolean) {
+    this._selectable = coerceBooleanProperty(value);
+  }


I think this selectable property is in the wrong class, but also not necessary for option and should probably be removed.

Removed selectable property

kara · 2017-07-19T20:13:15Z

src/lib/list/list.scss

@@ -139,10 +147,20 @@ $mat-dense-list-icon-size: 20px;
      $mat-list-icon-size
    );
  }
+
+  .mat-list-option {
+    @include mat-line-base(


Do you mean mat-list-item-base here? The mat-line-base mixin only takes one arg, so this will prevent it from building.

Yes! It should be mat-list-item-base

kara · 2017-07-19T20:16:22Z

src/lib/list/list-option.ts

+  selector: 'md-list-option, mat-list-option',
+  host: {
+    'role': 'option',
+    'class': 'mat-list-item, mat-list-option',


I think the comma is a typo? Don't want a comma between class names.

👍 change ['class': 'mat-list-item, mat-list-option',] to ['class': 'mat-list-item mat-list-option',]; Removed the comma between class names.

kara · 2017-07-19T20:19:55Z

src/lib/list/list-option.ts

+
+  /**
+   * Whether the ripple effect on click should be disabled. This applies only to list items that are
+   * part of a nav list. The value of `disableRipple` on the `md-nav-list` overrides this flag.


List-options are only on selection-lists, right? The comment talks about nav lists, so it looks like this might need to be updated?

updated the comments.

kara · 2017-07-19T20:24:02Z

src/lib/list/selection-list.ts

+
+  /**
+   * Whether or not this option is selectable. When a option is not selectable,
+   * it's selected state is always ignored.


Typo: it's -> its

kara · 2017-07-19T20:27:19Z

src/lib/list/selection-list.ts

+    option.onFocus.subscribe(() => {
+      let optionIndex: number = this.options.toArray().indexOf(option);
+
+      if (this._isValidIndex(optionIndex)) {


Why is the validity of the index necessary to check?

👍 Deleted unnecessaey this._isValidIndex(optionIndex) check

kara · 2017-07-19T20:32:07Z

src/lib/list/selection-list.ts

+   */
+  protected _addOption(option: MdListOption) {
+    // If we've already been subscribed to a parent, do nothing
+    if (this._subscribed.has(option)) {


Instead of keeping a list of options that were previously subscribed, it might be easier to merge all the option subscriptions into one stream. Then you can just switchMap to a new merged stream every time the option list changes. Check out how we do this in the autocomplete here and here.

👍 Finally make it work.
@kara @tinayuangao Thank you so much for your help~~! 🥇

sllethe

Comments Addressed 👍
Please take another look.

sllethe · 2017-07-20T17:44:24Z

src/lib/list/selection-list.ts

+  selector: 'md-selection-list, mat-selection-list',
+  host: {
+    'role': 'listbox',
+    '[attr.tabindex]': '_tabIndex',


👍 Set The tabindex of list option as -1 when the selection-list is disabled

sllethe · 2017-07-20T17:59:07Z

src/lib/list/list-option.ts

+  selector: 'md-list-option, mat-list-option',
+  host: {
+    'role': 'option',
+    'class': 'mat-list-item, mat-list-option',


👍 change ['class': 'mat-list-item, mat-list-option',] to ['class': 'mat-list-item mat-list-option',]; Removed the comma between class names.

sllethe · 2017-07-20T18:22:56Z

src/lib/list/list-option.ts

+
+  /**
+   * Whether the ripple effect on click should be disabled. This applies only to list items that are
+   * part of a nav list. The value of `disableRipple` on the `md-nav-list` overrides this flag.


updated the comments.

sllethe · 2017-07-20T18:24:35Z

src/lib/list/list.scss

@@ -139,10 +147,20 @@ $mat-dense-list-icon-size: 20px;
      $mat-list-icon-size
    );
  }
+
+  .mat-list-option {
+    @include mat-line-base(


Yes! It should be mat-list-item-base

sllethe · 2017-07-20T18:27:24Z

src/lib/list/selection-list.ts

+    '[attr.tabindex]': '_tabIndex',
+    'class': 'mat-selection-list',
+    '(focus)': 'focus()',
+    '(keydown)': 'keydown($event)'},


👍 Binded aria-disabled

sllethe · 2017-07-20T18:38:55Z

src/lib/list/selection-list.ts

+
+  /**
+   * Whether or not this option is selectable. When a option is not selectable,
+   * it's selected state is always ignored.


sllethe · 2017-07-20T19:00:08Z

src/lib/list/selection-list.ts

+  /** Whether the selection-list is disabled */
+  @Input()
+  get disabled() { return this._disabled; }
+  set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }


Applied mixin on disabled 👍

sllethe · 2017-07-20T19:06:55Z

src/lib/list/selection-list.ts

+  get selectable(): boolean { return this._selectable; }
+  set selectable(value: boolean) {
+    this._selectable = coerceBooleanProperty(value);
+  }


Removed selectable property

sllethe · 2017-07-20T19:14:33Z

src/lib/list/selection-list.ts

+    option.onFocus.subscribe(() => {
+      let optionIndex: number = this.options.toArray().indexOf(option);
+
+      if (this._isValidIndex(optionIndex)) {


👍 Deleted unnecessaey this._isValidIndex(optionIndex) check

sllethe · 2017-07-21T01:04:16Z

src/lib/list/selection-list.ts

+   */
+  protected _addOption(option: MdListOption) {
+    // If we've already been subscribed to a parent, do nothing
+    if (this._subscribed.has(option)) {


👍 Finally make it work.
@kara @tinayuangao Thank you so much for your help~~! 🥇

jelbourn

No other major comments on the implementation, just needs unit tests now

jelbourn · 2017-07-21T16:15:29Z

src/lib/list/selection-list.ts

+import {merge} from 'rxjs/operator/merge';
+import {Observable} from 'rxjs/Observable';
+import 'rxjs/add/operator/switchMap';
+import 'rxjs/add/operator/startWith';


The main components can't use the rxjs /add/ imports (and there should be a lint failure for doing so). You can use RxChain instead

It looks like you have some other lint errors as well; you can see them locally with gulp tslint

👍 Changed to use 'RxChain' instead of directly importing 'observer/add/'Changed to use 'RxChain' instead of directly importing 'observer/add/'

tinayuangao

LGTM

jelbourn · 2017-08-07T22:52:19Z

src/lib/list/_list-theme.scss

    .mat-subheader {
      @include mat-typography-level-to-styles($config, body-2);
    }
+
+    .mat-list-item-disabled {
+      background: #ededed;


This color needs to take whether we're in a light or dark theme into account; ideally it would come from the background color palette on the theme

changed to background-color: mat-color($background, disabled-list-option);

jelbourn · 2017-08-07T22:54:49Z

src/lib/list/list-option.ts

+  }
+
+  toggle(): void {
+    if (this._disabled == false && this.selectionList.disabled == false) {


Don't need to compare against false; you would just do

if (!this.disabled) { ... }

(the disabled getter already takes the selectionList state into account)

👍 changed to :

if (!this.disabled) { ... }

jelbourn · 2017-08-07T22:56:38Z

src/lib/list/list-option.ts

+    this._lineSetter = new MdLineSetter(this._lines, this._renderer, this._element);
+
+    if (this.selectionList.disabled) {
+      this._disabled = true;


It would be better not to mutate the option disabled state based on the parent state; this can be error prone, as it's easy to miss updating one when the other changes. It's better to rely on the disabled getter for the option to account for the parent state.

jelbourn · 2017-08-07T22:57:29Z

src/lib/list/list-option.ts

+  onFocus = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is selected. */
+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();


select -> selected

It seems selected has already been defined.

@Input() get selected() { return this._selected; } set selected( val: boolean) { this._selected = coerceBooleanProperty(val); }

or rename it to selectChange?

👍 Changed to selectChange

jelbourn · 2017-08-07T22:57:50Z

src/lib/list/list-option.ts

+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is deselected. */
+  @Output() deselect = new EventEmitter<MdSelectionListOptionEvent>();


deselect -> deselected

👍 Changed deselect -> deselected

jelbourn · 2017-08-07T22:59:32Z

src/lib/list/selection-list.ts

+  /**
+   * Map all the options' destroy event subscriptions and merge them into one stream.
+   */
+  onDestroySubscription(): Subscription {


This method should be private

👍 Changed it to 'private _onDestroySubscription()'

jelbourn · 2017-08-07T22:59:41Z

src/lib/list/selection-list.ts

+  /**
+   * Map all the options' onFocus event subscriptions and merge them into one stream.
+   */
+  onFocusSubscription(): Subscription {


This method should be private

👍 Changed to 'private _onFocusSubscription()'

jelbourn · 2017-08-07T22:59:56Z

src/lib/list/selection-list.ts

+  }
+
+  /** Passes relevant key presses to our key manager. */
+  keydown(event: KeyboardEvent) {


This should be internal (start with an _)

👍 Changed to '_keydown'

jelbourn · 2017-08-07T23:00:37Z

src/lib/list/selection-list.ts

+  }
+
+  /** Toggles the selected state of the currently focused option. */
+  protected _toggleSelectOnFocusedOption(): void {


This method should be private

👍 Changed to 'private'

jelbourn · 2017-08-07T23:03:10Z

src/lib/list/selection-list.spec.ts

+import {UP_ARROW, DOWN_ARROW, SPACE} from '../core/keyboard/keycodes';
+import {Platform} from '../core/platform/index';
+
+describe('test normal selection list with list option', () => {


You can wrap all of these child describe blocks in one top-level block:

describe('MdSelectionList', () => { ... });

Then, each describe block has a name that modifies the selection list, e.g.

describe('MdSelectionList', () => { describe('with multiple options', () => { ... }); describe('with one option', () => { ... }); describe('with disabled state', () => { ... }); });

This will make the test failures report like

MdSelectionList with one option should select an option

👍 Changed the structure like that~

sllethe

Comments Addressed 👍
Please take another look

sllethe · 2017-08-09T23:56:24Z

src/lib/list/list-option.ts

+  onFocus = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is selected. */
+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();


It seems selected has already been defined.

@Input() get selected() { return this._selected; } set selected( val: boolean) { this._selected = coerceBooleanProperty(val); }

sllethe · 2017-08-10T00:04:06Z

src/lib/list/list-option.ts

+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is deselected. */
+  @Output() deselect = new EventEmitter<MdSelectionListOptionEvent>();


👍 Changed deselect -> deselected

sllethe · 2017-08-10T00:12:17Z

src/lib/list/list-option.ts

+  @Output() deselect = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is destroyed. */
+  @Output() destroy = new EventEmitter<MdSelectionListOptionEvent>();


sllethe · 2017-08-10T00:15:38Z

src/lib/list/list-option.ts

+  }
+
+  toggle(): void {
+    if (this._disabled == false && this.selectionList.disabled == false) {


👍 changed to :

if (!this.disabled) { ... }

sllethe · 2017-08-10T00:15:51Z

src/lib/list/list-option.ts

+    this._lineSetter = new MdLineSetter(this._lines, this._renderer, this._element);
+
+    if (this.selectionList.disabled) {
+      this._disabled = true;


Changed to :

if (!this.disabled) { ... }

sllethe · 2017-08-10T00:36:23Z

src/lib/list/selection-list.spec.ts

+import {UP_ARROW, DOWN_ARROW, SPACE} from '../core/keyboard/keycodes';
+import {Platform} from '../core/platform/index';
+
+describe('test normal selection list with list option', () => {


👍 Changed the structure like that~

sllethe · 2017-08-10T00:37:26Z

src/lib/list/selection-list.ts

+  /**
+   * Map all the options' destroy event subscriptions and merge them into one stream.
+   */
+  onDestroySubscription(): Subscription {


👍 Changed it to 'private _onDestroySubscription()'

sllethe · 2017-08-10T00:39:10Z

src/lib/list/selection-list.ts

+  /**
+   * Map all the options' onFocus event subscriptions and merge them into one stream.
+   */
+  onFocusSubscription(): Subscription {


👍 Changed to 'private _onFocusSubscription()'

sllethe · 2017-08-10T00:41:33Z

src/lib/list/selection-list.ts

+  }
+
+  /** Passes relevant key presses to our key manager. */
+  keydown(event: KeyboardEvent) {


👍 Changed to '_keydown'

sllethe · 2017-08-10T00:42:22Z

src/lib/list/selection-list.ts

+  }
+
+  /** Toggles the selected state of the currently focused option. */
+  protected _toggleSelectOnFocusedOption(): void {


👍 Changed to 'private'

tinayuangao · 2017-08-10T00:49:54Z

src/lib/core/theming/_palette.scss

@@ -659,6 +659,7 @@ $mat-light-theme-background: (
  selected-button: map_get($mat-grey, 300),
  selected-disabled-button: map_get($mat-grey, 400),
  disabled-button-toggle: map_get($mat-grey, 200),
+        disabled-list-option: map_get($mat-grey, 200),


nit: indent

👍 Deleted indent

tinayuangao · 2017-08-10T00:54:40Z

src/lib/list/list-option.ts

+  onFocus = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is selected. */
+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();


or rename it to selectChange?

tinayuangao · 2017-08-10T00:54:52Z

src/lib/core/theming/_palette.scss

@@ -659,6 +659,7 @@ $mat-light-theme-background: (
  selected-button: map_get($mat-grey, 300),
  selected-disabled-button: map_get($mat-grey, 400),
  disabled-button-toggle: map_get($mat-grey, 200),
+        disabled-list-option: map_get($mat-grey, 200),


nit: indent

👍 Deleted indent

tinayuangao · 2017-08-10T00:56:43Z

src/lib/list/list-option.ts

+    this._lineSetter = new MdLineSetter(this._lines, this._renderer, this._element);
+
+    if (this.selectionList.disabled) {
+      this._disabled = true;


Better to delete if (this.selectionList.disabled) {...} part

tinayuangao · 2017-08-10T00:57:27Z

src/lib/list/list-option.ts

+  }
+
+  toggle(): void {
+    if (!this._disabled) {


Use this.disabled to consider the case when parent selectionList is disabled?

👍 Changed to this.disabled

tinayuangao · 2017-08-10T00:57:49Z

src/lib/list/list.scss

@@ -182,3 +212,5 @@ $mat-dense-list-icon-size: 20px;
    }
  }
 }
+


nit: too many empty lines

👍 Deleted empty lines

sllethe

Comments Addressed 👍
Please take another look~

sllethe · 2017-08-10T01:06:15Z

src/lib/core/theming/_palette.scss

@@ -659,6 +659,7 @@ $mat-light-theme-background: (
  selected-button: map_get($mat-grey, 300),
  selected-disabled-button: map_get($mat-grey, 400),
  disabled-button-toggle: map_get($mat-grey, 200),
+        disabled-list-option: map_get($mat-grey, 200),


👍 Deleted indent

sllethe · 2017-08-10T01:06:18Z

src/lib/core/theming/_palette.scss

@@ -659,6 +659,7 @@ $mat-light-theme-background: (
  selected-button: map_get($mat-grey, 300),
  selected-disabled-button: map_get($mat-grey, 400),
  disabled-button-toggle: map_get($mat-grey, 200),
+        disabled-list-option: map_get($mat-grey, 200),


👍 Deleted indent

sllethe · 2017-08-10T01:07:33Z

src/lib/list/list-option.ts

+  onFocus = new EventEmitter<MdSelectionListOptionEvent>();
+
+  /** Emitted when the option is selected. */
+  @Output() select = new EventEmitter<MdSelectionListOptionEvent>();


👍 Changed to selectChange

sllethe · 2017-08-10T17:06:49Z

src/lib/list/list-option.ts

+    this._lineSetter = new MdLineSetter(this._lines, this._renderer, this._element);
+
+    if (this.selectionList.disabled) {
+      this._disabled = true;


deleted if (this.selectionList.disabled) {...} part in toggle(), and put theif (this.selectionList.disabled) {...} part into a new function which is '_handleClick()'.

sllethe · 2017-08-10T17:07:56Z

src/lib/list/list-option.ts

+  }
+
+  toggle(): void {
+    if (!this._disabled) {


👍 Changed to this.disabled

sllethe · 2017-08-10T17:09:48Z

src/lib/list/list.scss

@@ -182,3 +212,5 @@ $mat-dense-list-icon-size: 20px;
    }
  }
 }
+


👍 Deleted empty lines

tinayuangao · 2017-08-10T17:38:59Z

LGTM. Please sync and rebase.

@Injectable

# This is the 1st commit message: add lib files for sticky-header add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify fix some code according to PR review comments change some format to pass TSlint check add '_' before private elements delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable refine code encapsulate 'set style for element' change @input() Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' add const STICK_START_CLASS and STICK_END_CLASS Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; encapsulate reset css style operation for sticky header. delete unnecessary gloable variables sticky-header-rebased add one test && fixed bug 'sticky-header not working on iPhone', and bug 'reshape too obviously when being sticked'. && made some change according to the design doc('md-stick --> cdkSticky') make the sticky element being scrolled up smoothly when being unsticked Add code to support optional parentRegion input add add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix delete unused files encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix clean unused files delete sticky-header delete styicky-heade demo delete sticky-header lib revert public_api delete sticky-header e2e files delete.firebasesrc revert revert revert revert add css styles for selection-list add selection-list structures in list.ts initial version of selection-list all src/lib code fix tslint error fix tslint error applied '[class.mat-list-item-content-reverse]="checkboxPosition == 'after'"', deleted comments in code Deleted 'MdSelectionListCssMatStyler' class Added doc for 'md-selection-option' Added binding expression for 'attr.aria-selected' and 'attr.aria-disabled' Added a value and a disabled @input() property Changed 'isSelected' to 'selected'. And added '@input(selected)' for it.(Using 'coerceBooleanProperty') changed '_slist' to 'selectionList' and deleted 'MdSelectionListCheckboxer' class Deleted navList in selection-list-option changed 'onchange()' to toggle(). And added doc for it checkedItems -> selectedOptions deleted 'pCheckbox' and store 'this' instead of HTMLelement in 'optionlist' optimized toggle() with 'this._selected = !this._selected' Added 'Tab' moves focus on selection-list, 'UP_ARROW' and 'DOWN_ARROW' moves focus within selection-list. And use 'SPACE' to select checkbox Added another empty line at the end of each file remove comment line in CSS file Deleted 'webkit-justify-content' in CSS file Deleted 'mat-list-item-content-reverse' to these existing styles; the 'mat-list-item-content-reverse' class should only have the extra styles you need to reverse the display Deleted 'mat-list-item-content' and 'mat-list-item-content-reverse' and '<a>' styles in '.mat-selection-list' Deleted unused 'mat-checkbox' class add ".mat-list-option" class in css file added 'mat-list-option' class fix toggle() disabled Deleted empty class Deleted unused import Removed 'if (this.selectable)' check moved styles in '.mat-selection-list .mat-list-item' to '.mat-list-option'. Deleted position, box-sizing, and height in '.mat-list-item-content-reverse' in list.scss file put md-selection-list and md-list-option in separate files(selection-list.ts and list-option.ts) # This is the commit message angular#2: nit # This is the commit message angular#3: correct imports in index.ts # This is the commit message angular#4: use ' @ContentChildren(MdListOption) options;' instead of QueryList

@Injectable

add chose parent add support to 'optional 'cdkStickyRegion' input ' add app-demo for sticky-header fix bugs and deleted unused tag id in HTML files modify fix some code according to PR review comments change some format to pass TSlint check add '_' before private elements delete @Injectable for StickyHeaderDirective. Because we do not need @Injectable refine code encapsulate 'set style for element' change @input() Delete 'Observable.fromEvent(this.upperScrollableContainer, 'scroll')' add const STICK_START_CLASS and STICK_END_CLASS Add doc for [cdkStickyRegion] and 'unstuckElement()'. Delete 'detach()' function, add its content into 'ngOnDestroy()'. change 'MdStickyHeaderModule' to 'CdkStickyHeaderModule'; encapsulate reset css style operation for sticky header. delete unnecessary gloable variables sticky-header-rebased add one test && fixed bug 'sticky-header not working on iPhone', and bug 'reshape too obviously when being sticked'. && made some change according to the design doc('md-stick --> cdkSticky') make the sticky element being scrolled up smoothly when being unsticked Add code to support optional parentRegion input add add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix delete unused files encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add unit test for sticky-header add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix encapsulate reset css style operation for sticky header. sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code master sticky-header-rebased Add code to support optional parentRegion input add e2e/sticky-header files e2e test clear code fix clean unused files delete sticky-header delete styicky-heade demo delete sticky-header lib revert public_api delete sticky-header e2e files delete.firebasesrc revert revert revert revert add css styles for selection-list add selection-list structures in list.ts initial version of selection-list all src/lib code fix tslint error fix tslint error applied '[class.mat-list-item-content-reverse]="checkboxPosition == 'after'"', deleted comments in code Deleted 'MdSelectionListCssMatStyler' class Added doc for 'md-selection-option' Added binding expression for 'attr.aria-selected' and 'attr.aria-disabled' Added a value and a disabled @input() property Changed 'isSelected' to 'selected'. And added '@input(selected)' for it.(Using 'coerceBooleanProperty') changed '_slist' to 'selectionList' and deleted 'MdSelectionListCheckboxer' class Deleted navList in selection-list-option changed 'onchange()' to toggle(). And added doc for it checkedItems -> selectedOptions deleted 'pCheckbox' and store 'this' instead of HTMLelement in 'optionlist' optimized toggle() with 'this._selected = !this._selected' Added 'Tab' moves focus on selection-list, 'UP_ARROW' and 'DOWN_ARROW' moves focus within selection-list. And use 'SPACE' to select checkbox Added another empty line at the end of each file remove comment line in CSS file Deleted 'webkit-justify-content' in CSS file Deleted 'mat-list-item-content-reverse' to these existing styles; the 'mat-list-item-content-reverse' class should only have the extra styles you need to reverse the display Deleted 'mat-list-item-content' and 'mat-list-item-content-reverse' and '<a>' styles in '.mat-selection-list' Deleted unused 'mat-checkbox' class add ".mat-list-option" class in css file added 'mat-list-option' class fix toggle() disabled Deleted empty class Deleted unused import Removed 'if (this.selectable)' check moved styles in '.mat-selection-list .mat-list-item' to '.mat-list-option'. Deleted position, box-sizing, and height in '.mat-list-item-content-reverse' in list.scss file put md-selection-list and md-list-option in separate files(selection-list.ts and list-option.ts) nit correct imports in index.ts use ' @ContentChildren(MdListOption) options;' instead of QueryList change to check 'focusedIndex != null'. Set The tabindex of list option as -1 when the selection-list is disabled change ['class': 'mat-list-item, mat-list-option',] to ['class': 'mat-list-item mat-list-option',]; Don't want a comma between class names. fix typo in comments fix Binded 'aria-disabled' for selection-list.ts rename to '_optionsChangeSubscription' typo Instead of defining the disabled getter/setter yourself, you can include it via mixin. Removed ' selectable' property Deleted unnecessary 'this._isValidIndex(optionIndex)' check import switchMap Added SwitchMerge nit revert revert revert2 revert3 r4 r5 r2 r3 r4 r5 revert fix rxjs import Changed to use 'RxChain' instead of directly importing 'observer/add/' delete unusd import Added exports for selectionlist fix tslint check fix tslint check fix tslint check Add tests for selection-list and list-option fix typo in test Added new test on native element's focus & blur Added check expect(selectList.selected.length).toBe(0);before the toggle nit Added test on desecting an option fix tslink check fix typo in doc refine docs fix 'What happens if the selection list is disabled later? Would the list option become disabled?' Changed all the test name to the format of 'should ...' rename Deleted unused '_tabOutSubscription' return RxChain.from(this.options.changes)... fix tsLint check fix tslint fix tslint error test can not pass on travel CI fix travelCI check try to fix nativeElement focus test revert fix test fix travis CI error fixing travis fixed~! update fix fix fix fix2 fix added test check on 'aria-selected' Added test checkou on 'aria-disabled' reformat structure changed to 'if (!fixture.componentInstance.isIE) { ... }' Change 'platform.SAFARI || platform.EDGE || platform.FIREFOX' to '!platform.IS_TRIDENT' fix travis check Added aris-disabled test fix travis test put disabled style in theme.scss use background color for disabled items deselect --> deselected destory -> destoryed optimize changed to "if (!this.disabled) { ... }" restructured test cases change function to private _keydown protected -> private indent change 'select' to 'selectChange' Better to delete if (this.selectionList.disabled) {...} part Use this.disabled to consider the case when parent selectionList is disabled? Deleted emty lines It would be better not to mutate the option disabled state based on the parent state; this can be error prone, as it's easy to miss updating one when the other changes. It's better to rely on the disabled getter for the option to account for the parent state.

sllethe · 2017-08-10T19:28:33Z

👍 synced and rebased

jelbourn

LGTM

jelbourn · 2017-08-10T23:24:15Z

@sllethe seems like the unit tests are failing

sllethe · 2017-08-12T05:12:32Z

fixed failing tests~

mmalerba · 2017-08-12T05:25:05Z

src/lib/list/selection-list.ts

+  constructor(private _renderer: Renderer2,
+              private _element: ElementRef,
+              private _changeDetector: ChangeDetectorRef,
+              @Self() @Optional() @Inject(forwardRef(() => MdSelectionList)) public selectionList: MdSelectionList) { }


No need for @Self() here, and Inject needs to be added to imports

👍 Deleted @Self() and added import for Inject

angular-automatic-lock-bot · 2019-09-06T21:01:56Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 6, 2017

sllethe requested review from jelbourn, tinayuangao and andrewseguin July 6, 2017 21:42

sllethe assigned jelbourn, tinayuangao and andrewseguin Jul 6, 2017

sllethe changed the title ~~Selection-list initial version al lcode~~ Selection-list initial version all code(without demo) Jul 6, 2017

jelbourn reviewed Jul 7, 2017

View reviewed changes

sllethe commented Jul 11, 2017

View reviewed changes

jelbourn reviewed Jul 11, 2017

View reviewed changes

sllethe commented Jul 12, 2017

View reviewed changes

tinayuangao reviewed Jul 12, 2017

View reviewed changes

sllethe force-pushed the selection-list-initialVersionAllcode branch from ab78883 to 8524538 Compare July 12, 2017 23:25

sllethe commented Jul 12, 2017

View reviewed changes

tinayuangao reviewed Jul 13, 2017

View reviewed changes

jelbourn reviewed Jul 14, 2017

View reviewed changes

sllethe commented Jul 14, 2017

View reviewed changes

tinayuangao changed the title ~~Selection-list initial version all code(without demo)~~ feat(selection-list): Selection-list initial version all code(without demo) Jul 17, 2017

jelbourn reviewed Jul 17, 2017

View reviewed changes

jelbourn reviewed Jul 19, 2017

View reviewed changes

kara reviewed Jul 19, 2017

View reviewed changes

sllethe force-pushed the selection-list-initialVersionAllcode branch from 43b88c6 to ba0d581 Compare July 20, 2017 19:13

sllethe commented Jul 21, 2017

View reviewed changes

sllethe force-pushed the selection-list-initialVersionAllcode branch 2 times, most recently from 9c2d9d6 to a97c924 Compare July 21, 2017 01:29

sllethe added the pr: needs review label Jul 21, 2017

jelbourn reviewed Jul 21, 2017

View reviewed changes

tinayuangao reviewed Jul 22, 2017

View reviewed changes

jelbourn reviewed Aug 7, 2017

View reviewed changes

sllethe commented Aug 10, 2017

View reviewed changes

tinayuangao reviewed Aug 10, 2017

View reviewed changes

sllethe commented Aug 10, 2017

View reviewed changes

sllethe added 3 commits August 10, 2017 12:22

fix after rebase

7f72853

sllethe force-pushed the selection-list-initialVersionAllcode branch from 2098fc6 to 7f72853 Compare August 10, 2017 19:25

fix

9a87582

jelbourn approved these changes Aug 10, 2017

View reviewed changes

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Aug 10, 2017

sllethe added 3 commits August 10, 2017 14:12

change 'grey, 1000' to 'black'

73975a9

remove unused import

3328d6e

fix test

748cfca

sllethe added 2 commits August 11, 2017 10:22

put selection-list.ts and list-option.ts into one file~ and fix the test

1d64782

fix google3

7a30b48

sllethe added 2 commits August 11, 2017 22:13

fix

022caa3

Add imports

63111b6

mmalerba reviewed Aug 12, 2017

View reviewed changes

sllethe added 2 commits August 11, 2017 22:29

fix

9f8a170

Deleted unused import

ae84d3b

mmalerba merged commit dccce1c into angular:master Aug 12, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

		@@ -0,0 +1,11 @@
		<div [ngClass]="(checkboxPosition == 'before')?'mat-list-item-content' : 'mat-list-item-content-reverse'" >
		<!--<div class="mat-list-item-content">-->


		@Input() checkboxPosition: 'before' \| 'after' = 'after';

		@ViewChild('autocheckbox') pCheckbox;


		/** Whether the label should appear after or before the checkbox. Defaults to 'after' */

		@Input() checkboxPosition: 'before' \| 'after' = 'after';

		@ContentChildren(MdLine) _lines: QueryList<MdLine>;

		/** Whether the label should appear after or before the checkbox. Defaults to 'after' */


		@Input() checkboxPosition: 'before' \| 'after' = 'after';

		/** Whether the checkbox is disabled. */


		let focusedIndex = this._keyManager.activeItemIndex;

		if (typeof focusedIndex === 'number' && this._isValidIndex(focusedIndex)) {

@@ @@ -182,3 +212,5 @@ $mat-dense-list-icon-size: 20px; @@
                   }
                 }
               }

feat(selection-list): Selection-list initial version all code(without demo) #5562

feat(selection-list): Selection-list initial version all code(without demo) #5562

Conversation

sllethe commented Jul 6, 2017

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

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

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

sllethe left a 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

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

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

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

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

sllethe left a 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

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

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

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

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