Skip to content

Cdk listbox control accessor #20071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
8c11c3a
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
7525a2c
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
26af9c9
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
67f3aad
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
61513bb
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
29b8513
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
12f1e80
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
bc8e583
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
fe81e8c
feat(listbox): added support for non-multiple listbox and aria active…
nielsr98 Jul 9, 2020
4414737
fix(listbox): formatted BUILD.bazel.
nielsr98 Jul 9, 2020
3bdfa83
feat(dev-app/listbox): added cdk listbox example to the dev-app.
nielsr98 Jul 15, 2020
cbf7c2d
feat(listbox): implemented ControlValueAccessor.
nielsr98 Jul 15, 2020
75c0dfa
nit(listbox): removed unused error class.
nielsr98 Jul 22, 2020
1d88375
fix(listbox): removed duplicate dep in dev-app build file.
nielsr98 Jul 22, 2020
047077a
fix(listbox): changed QueryList to array before iterating and fixed l…
nielsr98 Jul 22, 2020
5802c7d
fix(listbox): coreced array from values to ensure for loop does not i…
nielsr98 Jul 23, 2020
732d7c6
refactor(listbox): added a type T to CdkOption.
nielsr98 Jul 23, 2020
516354a
refactor(listbox): added tests for writeValue and setSelectedByValue.
nielsr98 Jul 24, 2020
eb2405d
fix(listbox): changed the coerceArray import path.
nielsr98 Jul 24, 2020
3b07bcc
nit(listbox): removed unused variables.
nielsr98 Jul 24, 2020
bf04326
fix(listbox): removed reference to undeclared variable.
nielsr98 Jul 24, 2020
4c4a24c
refactor(listbox): made listbox and option generic typed and added un…
nielsr98 Jul 28, 2020
c0aaac6
fix(listbox): removed unneccessary import and change detection refere…
nielsr98 Jul 28, 2020
4ccfbd6
nit(listbox): fixed formatting of BUILD file.
nielsr98 Jul 28, 2020
d00df03
fix(listbox): fixed lint errors.
nielsr98 Jul 28, 2020
d567cd0
fix(listbox): changed types of any to the generic type T.
nielsr98 Jul 29, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/cdk-experimental/listbox/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ ng_module(
"//src/cdk/a11y",
"//src/cdk/collections",
"//src/cdk/keycodes",
"@npm//@angular/forms",
],
)

Expand Down
78 changes: 70 additions & 8 deletions src/cdk-experimental/listbox/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {BooleanInput, coerceBooleanProperty} from '@angular/cdk/coercion';
import {SelectionChange, SelectionModel} from '@angular/cdk/collections';
import {defer, merge, Observable, Subject} from 'rxjs';
import {startWith, switchMap, takeUntil} from 'rxjs/operators';
import {ControlValueAccessor} from '@angular/forms';

let nextId = 0;

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

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

Choose a reason for hiding this comment

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

+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


@Output() readonly selectionChange: EventEmitter<OptionSelectionChangeEvent> =
new EventEmitter<OptionSelectionChangeEvent>();

Expand Down Expand Up @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

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

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>


_listKeyManager: ActiveDescendantKeyManager<CdkOption>;
_selectionModel: SelectionModel<CdkOption>;
_tabIndex = 0;

/** `View -> model callback called when select has been touched` */
_onTouched: () => void = () => {};

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

Choose a reason for hiding this comment

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

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


readonly optionSelectionChanges: Observable<OptionSelectionChangeEvent> = defer(() => {
const options = this._options;

Expand All @@ -197,7 +207,6 @@ export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {
private _multiple: boolean = false;
private _useActiveDescendant: boolean = true;
private _activeOption: CdkOption;

private readonly _destroyed = new Subject<void>();

@ContentChildren(CdkOption, {descendants: true}) _options: QueryList<CdkOption>;
Expand Down Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

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

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


ngOnInit() {
this._selectionModel = new SelectionModel<CdkOption>(this.multiple);
}
Expand Down Expand Up @@ -371,27 +382,33 @@ export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {
/** Selects the given option if the option and listbox aren't disabled. */
select(option: CdkOption) {
if (!this.disabled && !option.disabled) {
const wasSelected = option.selected;
option.select();

if (!wasSelected) {
this._emitChangeEvent(option);
this._updateSelectionModel(option);
}
}
}

/** Deselects the given option if the option and listbox aren't disabled. */
deselect(option: CdkOption) {
if (!this.disabled && !option.disabled) {
const wasSelected = option.selected;
option.deselect();

if (wasSelected) {
this._emitChangeEvent(option);
this._updateSelectionModel(option);
}
}
}

/** Sets the selected state of all options to be the given value. */
setAllSelected(isSelected: boolean) {
for (const option of this._options.toArray()) {
const wasSelected = option.selected;
isSelected ? this.select(option) : this.deselect(option);

if (wasSelected !== isSelected) {
this._emitChangeEvent(option);
this._updateSelectionModel(option);
}
}
}

Expand All @@ -400,6 +417,51 @@ export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {
this._listKeyManager.updateActiveItem(option);
}

/**
* 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 {
Copy link
Member

Choose a reason for hiding this comment

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

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

this._onChange = fn;
}

/**
* Saves a callback function to be invoked when the select is blurred
* by the user. Required to implement ControlValueAccessor.
*/
registerOnTouched(fn: () => {}): void {
this._onTouched = fn;
}

/** Sets the select's value. Required to implement ControlValueAccessor. */
writeValue(value: any): void {
if (this._options) {
this._setSelectionByValue(value);
}
}

/** Disables the select. Required to implement ControlValueAccessor. */
setDisabledState(isDisabled: boolean) {
this.disabled = isDisabled;
}

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

Choose a reason for hiding this comment

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

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

for (const option of this._options.toArray()) {
this.deselect(option);
}

for (const value of values) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

const correspondingOption = this._options.find((option: CdkOption) => {
return option.value != null && this.compareWith(option.value, value);
});

if (correspondingOption) {
this.select(correspondingOption);
}
}
}

static ngAcceptInputType_disabled: BooleanInput;
static ngAcceptInputType_multiple: BooleanInput;
static ngAcceptInputType_useActiveDescendant: BooleanInput;
Expand Down