feat(selection-list): support for ngModel #7456

devversion · 2017-10-01T17:45:18Z

Adds support for NgModel to the selection-list.

crisbeto

Mostly LGTM, a few nits.

crisbeto · 2017-10-02T18:27:58Z

src/lib/list/selection-list.ts

+  }
+
+  /** Returns the values of the selected options. */
+  private _getSelectedOptionValues(): string[] {


Consider inlining this since it's only used in one place and it's a one-liner?

I'd personally prefer keeping this as a function. It makes it the code more sorted with the _setOptionsFromValues method.

crisbeto · 2017-10-02T18:28:51Z

src/lib/list/selection-list.ts

+
+    values
+      .map(value => this._getOptionFromValue(value))
+      .filter(option => !!option)


Optional: if you want to be fancy, you can .filter(Boolean) here.

crisbeto · 2017-10-02T18:29:41Z

src/lib/list/selection-list.ts

+  }
+
+  /** Returns the option with the specified value. */
+  private _getOptionFromValue(value: string): MatListOption | undefined {


Call this _getOptionByValue? Also it's used only in one place so it could be inlined.

Sounds better. But I'd still like to keep it as a function. Otherwise it won't be that readable anymore.

crisbeto · 2017-10-02T18:30:20Z

src/lib/list/selection-list.ts

+  }
+
+  /** Implemented as part of ControlValueAccessor. */
+  registerOnTouched(fn: any): void {


The fn here and in registerOnChange is a () => any AFAIK.

Yeah we can add types here.

crisbeto · 2017-10-02T18:32:09Z

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

+      fixture.componentInstance.formControl.setValue(['opt2', 'opt3']);
+      fixture.detectChanges();
+
+      expect(listOptions[1].selected).toBe(true, 'Expected option to be selected.');


The two Expected option to be selected. are super useful since they're right after each other. Consider changing them to something like Expected second option to be selected. and Expected third option to be selected.?

crisbeto · 2017-10-02T18:34:01Z

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

+        .map(optionDebugEl => optionDebugEl.componentInstance);
+    });
+
+    it('should update the model if an option got selected', fakeAsync(() => {


Switch the name here to "should update the model if an option got selected programmatically? Otherwise it's very close to should update the model if an option got clicked`.

crisbeto

LGTM, blocked on #7334.

crisbeto · 2017-10-02T19:26:55Z

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

+      fixture.componentInstance.formControl.setValue(['opt2', 'opt3']);
+      fixture.detectChanges();
+
+      expect(listOptions[1].selected).toBe(true, 'Expected first option to be selected.');


These are wrong now. This is looking for the second option and the next one is looking for the third.

crisbeto · 2017-10-04T05:53:27Z

No longer blocked but needs a rebase.

jelbourn · 2017-10-04T19:27:10Z

src/lib/list/selection-list.ts

+      // available options. Also it can happen that the ControlValueAccessor has an initial value
+      // that should be used instead. Deferring the value change report to the next tick ensures
+      // that the form control value is not being overwritten.
+      Promise.resolve(() => this.selected && this.selectionList._reportValueChange());


@kara do you have any input on this?

devversion · 2017-10-06T12:40:35Z

Rebased. Waiting for @kara's feedback before re-applying merge-ready.

jelbourn

LGTM

jelbourn · 2017-10-26T05:24:41Z

Running into some meatadata extractor issue with NG_VALUE_ACCESSOR, not sure exactly what's up yet

josephperrott · 2017-11-06T23:18:44Z

src/lib/list/selection-list.ts


  /** The FocusKeyManager which handles focus. */
  _keyManager: FocusKeyManager<MatListOption>;

  /** The option components contained within this selection-list. */
  @ContentChildren(MatListOption) options: QueryList<MatListOption>;

+  /** Emits a change event whenever the selected state of an option changes. */
+  @Output() change: EventEmitter<MatSelectionListChange> =


This actually needs to be selectionChange rather than change

Looks like it got inadvertently renamed in this PR.

I intentionally named it change to be consistent with the other form elements supporting the ControlValueAccessor. I can switch it to selectionChange or also add a deprecated selectionChange output.

Let's go with selectionChange since that's what the mat-select now uses

devversion · 2017-11-18T10:29:17Z

@josephperrott @jelbourn Renamed the change event to selectionChange.

Also re-added the selectionChange event on the list option to avoid breaking changes. I marked that one as deprecated because we should encourage people to use the event on the list element instead.

Please have another look.

jelbourn · 2017-11-28T23:58:26Z

Setting back to merge-ready.

@tinayuangao if this fails on presubmit, can you help @devversion resolve any issues so that this can land for RC3?

* Adds support for NgModel to the selection-list. Fixes angular#6896

…option

kwalski · 2018-01-21T08:51:53Z

demo app/docs need to be updated to show usage of ngModel with selection-list

SvenBudak · 2018-03-28T19:13:18Z

is there any update for the documentation planed?

angular-automatic-lock-bot · 2019-09-08T14:01:08Z

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.}

devversion added the pr: needs review label Oct 1, 2017

devversion assigned jelbourn and crisbeto Oct 1, 2017

devversion requested review from crisbeto and jelbourn as code owners October 1, 2017 17:45

googlebot added the cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla label Oct 1, 2017

devversion force-pushed the feat/selection-list-model branch from e918191 to 92d65d8 Compare October 2, 2017 16:10

crisbeto reviewed Oct 2, 2017

View reviewed changes

crisbeto approved these changes Oct 2, 2017

View reviewed changes

angular deleted a comment from googlebot Oct 2, 2017

googlebot added cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla and removed cla: yes PR author has agreed to Google's Contributor License Agreement labels Oct 2, 2017

angular deleted a comment from googlebot Oct 2, 2017

devversion added cla: yes PR author has agreed to Google's Contributor License Agreement and removed cla: no PR author must sign Google's Contributor License Agreement: https://opensource.google.com/docs/cla labels Oct 2, 2017

crisbeto added action: merge The PR is ready for merge by the caretaker and removed blocked This issue is blocked by some external factor, such as a prerequisite PR labels Oct 4, 2017

crisbeto added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Oct 4, 2017

devversion force-pushed the feat/selection-list-model branch from 1cc3f87 to b9654bd Compare October 4, 2017 14:16

jelbourn reviewed Oct 4, 2017

View reviewed changes

devversion added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase action: merge The PR is ready for merge by the caretaker labels Oct 5, 2017

devversion force-pushed the feat/selection-list-model branch from b9654bd to 6cae628 Compare October 6, 2017 12:40

ghost mentioned this pull request Oct 11, 2017

How to use angular material Selection List #7712

Closed

devversion force-pushed the feat/selection-list-model branch from 6cae628 to a72026d Compare October 14, 2017 12:02

devversion added P2 The issue is important to a large percentage of users, with a workaround P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful and removed P2 The issue is important to a large percentage of users, with a workaround labels Oct 14, 2017

jelbourn approved these changes Oct 25, 2017

View reviewed changes

jelbourn added action: merge The PR is ready for merge by the caretaker presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Oct 25, 2017

josephperrott reviewed Nov 6, 2017

View reviewed changes

josephperrott removed action: merge The PR is ready for merge by the caretaker presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged labels Nov 6, 2017

devversion force-pushed the feat/selection-list-model branch from a72026d to 006d8ac Compare November 18, 2017 10:05

jelbourn added this to the 5.0 milestone Nov 19, 2017

This was referenced Nov 20, 2017

Setting mat-list-option selected attribute programatically triggers selectionChange event #8550

Closed

Unable to get selectonChange event fired on mat-list-option under mat-selection-list #8555

Closed

jelbourn added the action: merge The PR is ready for merge by the caretaker label Nov 28, 2017

devversion added 3 commits December 5, 2017 18:19

feat(selection-list): support for ngModel

9fc9114

* Adds support for NgModel to the selection-list. Fixes angular#6896

Rename to selectionChange. Add deprecated selectionChange event on …

18a54e6

…option

Reintroduce MatListOptionChange for deprecation

ce51421

devversion force-pushed the feat/selection-list-model branch from 006d8ac to ce51421 Compare December 5, 2017 17:40

mmalerba merged commit 3fbb28a into angular:master Dec 5, 2017

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

feat(selection-list): support for ngModel #7456

feat(selection-list): support for ngModel #7456

Uh oh!

Conversation

devversion commented Oct 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto commented Oct 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented Oct 6, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Oct 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devversion commented Nov 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Nov 28, 2017

Uh oh!

kwalski commented Jan 21, 2018

Uh oh!

SvenBudak commented Mar 28, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!

devversion commented Oct 1, 2017 •

edited

Loading

devversion commented Nov 18, 2017 •

edited

Loading