Cdk listbox control accessor #20071

nielsr98 · 2020-07-22T14:58:35Z

Implemented ControlValueAccessor in listbox, which entailed adding a value property and implementing several required methods. This is done to allow for listbox to be used with form control.

…iles.

…descendant.

…int errors.

crisbeto · 2020-07-22T18:50:24Z

src/cdk-experimental/listbox/listbox.ts

@@ -67,6 +68,9 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
    this._disabled = coerceBooleanProperty(value);
  }

+  /** The form value of the option. */
+  @Input() value: any;


Maybe the option should be generic so people can type the value if they want to? E.g.

class CdkOption<T = any> { value: T; }

Hmm this seems like a good idea. Do you know if there's a reason why MatOption doesn't do this?

+1, MatOption didn't do this because we didn't know any better back when we wrote it. I would, though, use unknown instead of any for the default

src/cdk-experimental/listbox/listbox.ts

crisbeto · 2020-07-22T18:53:32Z

src/cdk-experimental/listbox/listbox.ts

+      this.deselect(option);
+    }
+
+    for (const value of values) {


Won't this break down if the value isn't an array? Based on the typing it could be a string which will work inside the for...of loop, but will loop through character by character.

I neglected to test that out. I'll test out different methods and find a way to handle it. Thanks.

jelbourn

+1 to Kristiyan's comments; once those are good you should be able to add the unit tests

jelbourn · 2020-07-22T23:49:21Z

src/cdk-experimental/listbox/listbox.ts

@@ -67,6 +68,9 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
    this._disabled = coerceBooleanProperty(value);
  }

+  /** The form value of the option. */
+  @Input() value: any;


+1, MatOption didn't do this because we didn't know any better back when we wrote it. I would, though, use unknown instead of any for the default

…terate through string characters.

nielsr98 · 2020-07-24T21:06:16Z

Addressed feedback and added some unit tests.

jelbourn · 2020-07-24T22:37:07Z

src/cdk-experimental/listbox/listbox.ts

+  _onTouched: () => void = () => {};
+
+  /** `View -> model callback called when value changes` */
+  _onChange: (value: any) => void = () => {};


Suggested change

_onChange: (value: any) => void = () => {};

_onChange: (value: T) => void = () => {};

jelbourn · 2020-07-24T22:37:27Z

src/cdk-experimental/listbox/listbox.ts

@@ -235,6 +244,8 @@ export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {
    this._useActiveDescendant = coerceBooleanProperty(shouldUseActiveDescendant);
  }

+  @Input() compareWith: (o1: any, o2: any) => boolean = (a1, a2) => a1 === a2;


Suggested change

@Input() compareWith: (o1: any, o2: any) => boolean = (a1, a2) => a1 === a2;

@Input() compareWith: (o1: T, o2: T) => boolean = (a1, a2) => a1 === a2;

jelbourn · 2020-07-24T22:37:38Z

src/cdk-experimental/listbox/listbox.ts

+   * Saves a callback function to be invoked when the select's value
+   * changes from user input. Required to implement ControlValueAccessor.
+   */
+  registerOnChange(fn: (value: any) => void): void {


Suggested change

registerOnChange(fn: (value: any) => void): void {

registerOnChange(fn: (value: T) => void): void {

jelbourn · 2020-07-24T22:39:29Z

src/cdk-experimental/listbox/listbox.ts

@@ -178,12 +182,18 @@ export class CdkOption implements ListKeyManagerOption, Highlightable {
      '[attr.aria-activedescendant]': '_getAriaActiveDescendant()'
    }
 })
-export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {
+export class CdkListbox implements AfterContentInit, OnDestroy, OnInit, ControlValueAccessor {


Suggested change

export class CdkListbox implements AfterContentInit, OnDestroy, OnInit, ControlValueAccessor {

export class CdkListbox<T> implements AfterContentInit, OnDestroy, OnInit, ControlValueAccessor {

And all references to CdkOption throughout should be CdkOption<T>

jelbourn · 2020-07-24T22:40:44Z

src/cdk-experimental/listbox/listbox.ts

+  }
+
+  /** Sets the select's value. Required to implement ControlValueAccessor. */
+  writeValue(values: any | any[]): void {


Suggested change

writeValue(values: any | any[]): void {

writeValue(value: T | T[]): void {

jelbourn · 2020-07-24T22:40:57Z

src/cdk-experimental/listbox/listbox.ts

+  }
+
+  /** Selects an option that has the corresponding given value. */
+  private _setSelectionByValue(values: any | any[]) {


Suggested change

private _setSelectionByValue(values: any | any[]) {

private _setSelectionByValue(values: T| T[]) {

jelbourn · 2020-07-24T22:44:32Z

src/cdk-experimental/listbox/listbox.ts

+    }
+
+    const valuesArray = coerceArray(values);
+    console.log(valuesArray);


jelbourn · 2020-07-24T22:47:11Z

src/cdk-experimental/listbox/listbox.spec.ts

@@ -655,3 +738,23 @@ class ListboxActiveDescendant {
    this.focusedOption = option;
  }
 }
+


I would also add test cases for actually using the listbox with a FormControl, and then testing that the interaction works as expected via that FormControl

…it tests for using form control.

…nce object.

nielsr98 · 2020-07-28T17:36:11Z

Feedback address. Ready for review and check on the changes.

jelbourn

Looks like there's a few lingering any types in the listbox that can be T

jelbourn

LGTM

angular-automatic-lock-bot · 2020-08-29T00:11:18Z

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

nielsr98 added 14 commits July 22, 2020 10:35

build: Added required files to listbox directory.

8c11c3a

build: added listbox option directive and renamed listbox directive f…

7525a2c

…iles.

build: Added required files to listbox directory.

26af9c9

build: added listbox option directive and renamed listbox directive f…

67f3aad

…iles.

build: Added required files to listbox directory.

61513bb

build: added listbox option directive and renamed listbox directive f…

29b8513

…iles.

build: Added required files to listbox directory.

12f1e80

build: added listbox option directive and renamed listbox directive f…

bc8e583

…iles.

feat(listbox): added support for non-multiple listbox and aria active…

fe81e8c

…descendant.

fix(listbox): formatted BUILD.bazel.

4414737

feat(dev-app/listbox): added cdk listbox example to the dev-app.

3bdfa83

feat(listbox): implemented ControlValueAccessor.

cbf7c2d

nit(listbox): removed unused error class.

75c0dfa

fix(listbox): removed duplicate dep in dev-app build file.

1d88375

nielsr98 requested a review from jelbourn as a code owner July 22, 2020 14:58

nielsr98 requested a review from scriby July 22, 2020 14:58

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

fix(listbox): changed QueryList to array before iterating and fixed l…

047077a

…int errors.

nielsr98 added the target: minor This PR is targeted for the next minor release label Jul 22, 2020

crisbeto reviewed Jul 22, 2020

View reviewed changes

jelbourn reviewed Jul 22, 2020

View reviewed changes

nielsr98 added 6 commits July 23, 2020 16:38

fix(listbox): coreced array from values to ensure for loop does not i…

5802c7d

…terate through string characters.

refactor(listbox): added a type T to CdkOption.

732d7c6

refactor(listbox): added tests for writeValue and setSelectedByValue.

516354a

fix(listbox): changed the coerceArray import path.

eb2405d

nit(listbox): removed unused variables.

3b07bcc

fix(listbox): removed reference to undeclared variable.

bf04326

jelbourn reviewed Jul 24, 2020

View reviewed changes

refactor(listbox): made listbox and option generic typed and added un…

4c4a24c

…it tests for using form control.

nielsr98 added 3 commits July 28, 2020 11:27

fix(listbox): removed unneccessary import and change detection refere…

c0aaac6

…nce object.

nit(listbox): fixed formatting of BUILD file.

4ccfbd6

fix(listbox): fixed lint errors.

d00df03

jelbourn reviewed Jul 29, 2020

View reviewed changes

fix(listbox): changed types of any to the generic type T.

d567cd0

jelbourn approved these changes Jul 29, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker labels Jul 29, 2020

jelbourn merged commit 5f5f5aa into angular:master Jul 29, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Aug 29, 2020

	_onChange: (value: any) => void = () => {};
	_onChange: (value: T) => void = () => {};

	@Input() compareWith: (o1: any, o2: any) => boolean = (a1, a2) => a1 === a2;
	@Input() compareWith: (o1: T, o2: T) => boolean = (a1, a2) => a1 === a2;

	registerOnChange(fn: (value: any) => void): void {
	registerOnChange(fn: (value: T) => void): void {

	export class CdkListbox implements AfterContentInit, OnDestroy, OnInit, ControlValueAccessor {
	export class CdkListbox<T> implements AfterContentInit, OnDestroy, OnInit, ControlValueAccessor {

	writeValue(values: any \| any[]): void {
	writeValue(value: T \| T[]): void {

	private _setSelectionByValue(values: any \| any[]) {
	private _setSelectionByValue(values: T\| T[]) {

Cdk listbox control accessor #20071

Cdk listbox control accessor #20071

Uh oh!

Conversation

nielsr98 commented Jul 22, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nielsr98 commented Jul 24, 2020

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!

nielsr98 commented Jul 28, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Aug 29, 2020

Uh oh!

Uh oh!