Combobox base #20211

nielsr98 · 2020-08-06T05:37:34Z

Added most base features of combobox and panel. By default this combobox will always trigger on click. More unit tests are required. Only basic unit tests are implemented thus far.

…iles.

…el, and some tests.

jelbourn

Got most of the way through before my next meeting- will finish looking later today!

jelbourn · 2020-08-07T15:53:50Z

src/cdk-experimental/combobox/combobox-panel.ts


 @Directive({
  selector: 'ng-template[cdkComboboxPanel]',
  exportAs: 'cdkComboboxPanel',
+  host: {
+    'aria-controls': 'contentId',


I don't think these aria attributes would go on the panel, they would go on the combobox trigger. The values are only necessary in the panel so that it can pass them back to the trigger

So I guess using a Subject for these two as well would be appropriate? Then on register of the content the panel updated the Subject and the trigger can grab the new value.

jelbourn · 2020-08-07T15:57:44Z

src/cdk-experimental/combobox/combobox-panel.ts

 })
 export class CdkComboboxPanel<T = unknown> {

+  valueUpdated: Subject<T> = new Subject<T>();
+  contentId: string = '';
+  contentType: string = '';


jelbourn · 2020-08-07T15:58:06Z

src/cdk-experimental/combobox/combobox-panel.ts

+
+  constructor(readonly _templateRef: TemplateRef<unknown>) {}
+
+  closePanel(data?: T) {


Add JsDoc for public methods
(here and below)

jelbourn · 2020-08-07T15:59:25Z

src/cdk-experimental/combobox/combobox-panel.ts

+  _registerContent(contentId: string, contentType: string) {
+    this.contentId = contentId;
+    if (contentType !== 'listbox' && contentType !== 'dialog') {
+      throw Error('CdkComboboxPanel content must be either a listbox or dialog');


I might tweak the wording here that this only currently supports "listbox" or "dialog"

Gotcha will do.

jelbourn · 2020-08-07T16:01:20Z

src/cdk-experimental/combobox/combobox.ts

-    'role': 'combobox'
+    'role': 'combobox',
+    '(click)': 'toggle()',
+    '[attr.aria-disabled]': 'disabled'


The aria-haspopup and aria-controls should go here.

I also realized just now that there's a mode of interaction where this element controls a listbox while focus remains on the combobox element and it uses aria-activedescendant to indicate which listbox option is active. Seems like something to consider later on

jelbourn · 2020-08-07T16:02:52Z

src/cdk-experimental/combobox/combobox.ts

+  @Input('triggerFor')
+  get comboboxPanel(): CdkComboboxPanel<T> | undefined {
+    return this._comboboxPanel;
+  }
+  set comboboxPanel(panel: CdkComboboxPanel<T> | undefined) {
+    this._comboboxPanel = panel;
+  }
+  private _comboboxPanel: CdkComboboxPanel<T> | undefined;


Suggested change

@Input('triggerFor')

get comboboxPanel(): CdkComboboxPanel<T> | undefined {

return this._comboboxPanel;

}

set comboboxPanel(panel: CdkComboboxPanel<T> | undefined) {

this._comboboxPanel = panel;

}

private _comboboxPanel: CdkComboboxPanel<T> | undefined;

@Input('cdkComboboxTriggerFor')

get panel(): CdkComboboxPanel<T> | undefined {

return this._panel;

}

set panel(panel: CdkComboboxPanel<T> | undefined) {

this._panel= panel;

}

private _panel: CdkComboboxPanel<T> | undefined;

jelbourn · 2020-08-07T16:04:17Z

src/cdk-experimental/combobox/combobox.ts

+  @Input()
+  get value(): T {
+    return this._value;
+  }
+  set value(val: T) {
+    this._value = val;
+  }
+  private _value: T;


Suggested change

@Input()

get value(): T {

return this._value;

}

set value(val: T) {

this._value = val;

}

private _value: T;

@Input()

value: T;

I'd avoid adding the getter/setter until you need it

jelbourn · 2020-08-07T16:04:56Z

src/cdk-experimental/combobox/combobox.ts

+  get disabled(): boolean {
+    return this._disabled;
+  }
+  set disabled(value: boolean) {
+    this._disabled = coerceBooleanProperty(value);
+  }
+  private _disabled: boolean = false;


optional: you can condense simple getter/setters like this

Suggested change

get disabled(): boolean {

return this._disabled;

}

set disabled(value: boolean) {

this._disabled = coerceBooleanProperty(value);

}

private _disabled: boolean = false;

get disabled(): boolean { return this._disabled; }

set disabled(value: boolean) { this._disabled = coerceBooleanProperty(value); }

private _disabled: boolean = false;

jelbourn · 2020-08-07T16:05:34Z

src/cdk-experimental/combobox/combobox.ts

+    ];
+  }
+
+  private _getPortal() {


Suggested change

private _getPortal() {

private _getPanelContent() {

…opup and aria-controls to the combobox not the panel.

… function.

jelbourn · 2020-08-07T21:25:19Z

src/cdk-experimental/combobox/combobox-panel.ts

@@ -6,12 +6,40 @@
 * found in the LICENSE file at https://angular.io/license
 */

-import {Directive} from '@angular/core';
+export type ContentType = 'false' | 'true' | 'menu' | 'listbox' | 'tree' | 'grid' | 'dialog';


jelbourn · 2020-08-07T21:28:43Z

src/cdk-experimental/combobox/combobox.ts

+  get comboboxPanel(): CdkComboboxPanel<T> | undefined { return this._panel; }
+  set comboboxPanel(panel: CdkComboboxPanel<T> | undefined) { this._panel = panel; }


Suggested change

get comboboxPanel(): CdkComboboxPanel<T> | undefined { return this._panel; }

set comboboxPanel(panel: CdkComboboxPanel<T> | undefined) { this._panel = panel; }

get panel(): CdkComboboxPanel<T> | undefined { return this._panel; }

set panel(panel: CdkComboboxPanel<T> | undefined) { this._panel = panel; }

jelbourn · 2020-08-07T21:30:42Z

src/cdk-experimental/combobox/combobox.ts

+    return this._panelContent;
+  }
+
+  private _coerceOpenActionProperty(input: any): OpenAction[] {


Rather than any, this can be the union of all the types this can handle

jelbourn · 2020-08-07T22:23:21Z

src/cdk-experimental/combobox/combobox.ts

+    const actions: OpenAction[] = [];
+
+    const inputArray = coerceArray(input);
+    for (const action of inputArray) {
+      if (action !== 'focus' && action !== 'click' && action !== 'downKey' && action !== 'toggle') {
+        throw Error('Not a valid open action.');
+      } else {
+        actions.push(action);
+      }
+    }
+
+    return actions;


I would just have this whole function be:

Suggested change

const actions: OpenAction[] = [];

const inputArray = coerceArray(input);

for (const action of inputArray) {

if (action !== 'focus' && action !== 'click' && action !== 'downKey' && action !== 'toggle') {

throw Error('Not a valid open action.');

} else {

actions.push(action);

}

}

return actions;

return input;

And add the rest of the behavior when you add support for splitting a string

jelbourn · 2020-08-07T22:41:57Z

src/cdk-experimental/combobox/combobox.ts

+    '[attr.aria-controls]': 'contentId',
+    '[attr.aria-haspopup]': 'contentType'


Add unit tests for these attributes?

…impler for this PR.

jelbourn

LGTM

angular-automatic-lock-bot · 2020-09-12T00:10:16Z

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 13 commits August 6, 2020 01:32

build: Added required files to listbox directory.

06737c4

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

24c1ca6

…iles.

build: Added required files to listbox directory.

86d7b58

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

2e8ae02

…iles.

build: Added required files to listbox directory.

4c290e3

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

43a3d93

…iles.

build: Added required files to listbox directory.

906dea3

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

c289a7d

…iles.

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

4ac224b

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

df2e62f

fix(listbox): deleted unused files.

f72129c

feat(combobox): added all basic features to combobox and combobox pan…

a739263

…el, and some tests.

feat(combobox): finished basic tests and clean up.

0d0dea8

nielsr98 requested a review from jelbourn as a code owner August 6, 2020 05:37

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

nielsr98 added 3 commits August 6, 2020 01:52

fix(combobox): fixed the import path for coercion.

a2a9c9b

fix(combobox): fixed import path for combobox panel.

f5314db

fix(combobox): fixed lint errors throughout.

cc2cb62

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

nielsr98 added 2 commits August 6, 2020 09:56

refactor(combobox): removed unused panel test file.

5922cc9

refactor(combobox): changed import path for panel in combobox.

a768b2c

jelbourn requested a review from crisbeto August 7, 2020 15:52

jelbourn reviewed Aug 7, 2020

View reviewed changes

nielsr98 added 4 commits August 7, 2020 16:51

refactor(combobox): cleaned up Inputs in combobox and moved aria-hasp…

10d038c

…opup and aria-controls to the combobox not the panel.

refactor(combobox): added jsdoc to the public functions.

a0e5d78

fix(combobox): removed duplicate ContentType type and getPanelContent…

a6f8750

… function.

refactor(combobox): made contentId and contentType of combobox public.

856b5c3

jelbourn reviewed Aug 7, 2020

View reviewed changes

nielsr98 added 2 commits August 10, 2020 17:46

refactor(combobox): changed names and made coerceOpenActionProperty s…

f79781c

…impler for this PR.

fix(combobox): updated syntax for casting.

681c9d7

nielsr98 added 2 commits August 10, 2020 18:13

refactor(combobox): changed casting syntax back.

36b297a

fix(combobox): fixed trailing whitespace.

9673d33

jelbourn approved these changes Aug 10, 2020

View reviewed changes

jelbourn added action: merge The PR is ready for merge by the caretaker merge safe labels Aug 10, 2020

andrewseguin added the merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed label Aug 12, 2020

andrewseguin merged commit 6184328 into angular:master Aug 12, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 12, 2020


		constructor(readonly _templateRef: TemplateRef<unknown>) {}

		closePanel(data?: T) {

	export type ContentType = 'false' \| 'true' \| 'menu' \| 'listbox' \| 'tree' \| 'grid' \| 'dialog';
	export type AriaHasPopupValue = 'false' \| 'true' \| 'menu' \| 'listbox' \| 'tree' \| 'grid' \| 'dialog';

		get comboboxPanel(): CdkComboboxPanel<T> \| undefined { return this._panel; }
		set comboboxPanel(panel: CdkComboboxPanel<T> \| undefined) { this._panel = panel; }

		'[attr.aria-controls]': 'contentId',
		'[attr.aria-haspopup]': 'contentType'

Combobox base #20211

Combobox base #20211

Uh oh!

Conversation

nielsr98 commented Aug 6, 2020

Uh oh!

jelbourn 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!

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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Sep 12, 2020

Uh oh!

Uh oh!