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.

Sign up for GitHub

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

Jump to bottom

Combobox open actions #20306

Merged

andrewseguin merged 33 commits into angular:master from nielsr98:combobox-open-actions

Aug 14, 2020

Contributor

nielsr98 commented Aug 13, 2020

New features include:

Logic to handle multiple different open actions
Coercion function for string -> open action
Many additional tests

nielsr98 requested a review from jelbourn as a code owner

August 13, 2020 21:02

nielsr98 requested a review from scriby

August 13, 2020 21:02

googlebot added the cla: yes label

jelbourn reviewed

View reviewed changes

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

                 closePanel(data?: T) {
                   this.valueUpdated.next(data);
                 }
+                focusContent() {
+                  document.getElementById(this.contentId)?.focus();

Member

jelbourn Aug 13, 2020

I would add a TODO here to use an injected document and a separate TODO to potentially switch this to use focus trapping from cdk/a11y

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

-                constructor(readonly _templateRef: TemplateRef<unknown>) {}
+                constructor(
+                  readonly _templateRef: TemplateRef<unknown>,
+                  readonly _elementRef: ElementRef<HTMLElement>

Member

jelbourn Aug 13, 2020

I think you can drop this elementRef now

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

                   this.panelValueChanged.complete();
                 }
+                onClick() {

Member

jelbourn Aug 13, 2020

It would probably make sense to make one method like _handleInteraction(action) and then set up the event handlers like '(click)': '_handleInteraction("click")', etc.

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

+                  }
+                }
+                @HostListener('document:click', ['$event'])

Member

jelbourn Aug 13, 2020

Prefer adding this to the host object

host: {
  '(document:click)': '_attemptClose($event)',
}

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

                 private _coerceOpenActionProperty(input: string | OpenAction[]): OpenAction[] {
                   let actions: OpenAction[] = [];
+                  const viableActions = ['focus', 'click', 'downKey', 'toggle'];

Member

jelbourn Aug 13, 2020

I would change this to a file-level constant

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

Comment on lines 243 to 257

                   let actions: OpenAction[] = [];
+                  const viableActions = ['focus', 'click', 'downKey', 'toggle'];
                   if (typeof input === 'string') {
-                    actions.push(input as OpenAction);
+                    const tokens = input.trim().split(/[ ,]+/);
+                    for (const token of tokens) {
+                      if (viableActions.indexOf(token) === -1) {
+                        throw Error(`${token} is not a supported open action`);
+                      }
+                      actions.push(token as OpenAction);
+                    }
                   } else {
-                    actions = input;
+                    actions = coerceArray(input);
                   }
                   return actions;

Member

jelbourn Aug 13, 2020

let actions = typeof input === 'string' ? input.trim().split(/[ ,]+/) : input;
if (isDevMode() && actions.some(a => allowedOpenActions.indexOf(a) === -1)) {
  throw Error(`${input} is not a support open action for CdkCombobox`);
}
return actions;

We'd want to do the sanity check even if the value isn't a string
We'd only want to do the sanity check in dev mode
I think the else isn't doing anything since it's already an array if it's not a string

Contributor Author

nielsr98 Aug 14, 2020

I see this makes sense. The some function helps condense it too.

Matausi29 approved these changes

View reviewed changes

Matausi29 approved these changes

View reviewed changes

Matausi29 left a comment

jelbourn reviewed

View reviewed changes

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

+                  }
+                }
+                _handleInteractions(interaction: string) {

Member

jelbourn Aug 14, 2020

Suggested change

      
              _handleInteractions(interaction: string) {
          
              _handleInteractions(interaction: OpenAction) {

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

                   this.panelValueChanged.complete();
                 }
+                keydown(event: KeyboardEvent) {

Member

jelbourn Aug 14, 2020

Suggested change

      
              keydown(event: KeyboardEvent) {
          
              _keydown(event: KeyboardEvent) {

Should have an underscore since this isn't a public API

src/cdk-experimental/combobox/combobox.spec.ts Outdated

+                  'tabIndex': '-1'
+                }
+              })
+              export class DialogContent<V> implements OnInit {

Member

jelbourn Aug 14, 2020

I might make this FakeDialogContent just so it doesn't come up for IDE shortcuts like shift+shift when looking for dialog stuff

jelbourn approved these changes

View reviewed changes

Member

jelbourn left a comment

LGTM

nielsr98 added 22 commits

August 14, 2020 13:11


          build: Added required files to listbox directory.

bfdb156


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

f8f6399

…iles.


          build: Added required files to listbox directory.

b45c6df


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

966225f

…iles.


          build: Added required files to listbox directory.

3ae5410


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

46f537e

…iles.


          build: Added required files to listbox directory.

aa3abdb


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

6ea67c5

…iles.


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

db690d6


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

dea5915


          fix(listbox): deleted unused files.

97eee8b


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

78bfc99

…impler for this PR.


          fix(combobox): updated syntax for casting.

3b955df


          refactor(combobox): changed casting syntax back.

2f31abf


          fix(combobox): fixed trailing whitespace.

ab86d96


          refactor(combobox): improved coerceOpenActions function to handle whi…

a3e6b05

…tespace separated strings as input.


          fix(combobox): split the coerceOpenActions on comma or space.

9e06f83


          feat(combobox): added aria-expanded and aria-owns traits.


          refactor(combobox): minor refactor.

ac7b942


          feat(combobox): added logic to close popup on click outside of combobox.

5d7f8d5


          test(combobox): added many tests for combobox open action features.

e0ddc23


          test(combobox): added back two failing tests, focus and sending input…

7c5b802

… value to combobox.

nielsr98 added 10 commits

August 14, 2020 13:11


          refactor(combobox): further tests on focus.

517b42b


          refactor(combobox): added tabindex to allow focusing on content, and …

60d0079

…fixed the setTextContent function.


          fix(combobox): removed separate coerceArray import.

64dd2f6


          fix(combobox): lint errors.

7ffc07f


          refactor(combobox): only throw error in coercing open action in dev m…

1b28ca7

…ode, condense click and focus handlers into one.


          nit(combobox): removed whitespace.

d04feb9


          fix(combobox): removed unused imports.

4671b89


          feat(combobox): added combobox popup directive to handle registering …

75c7825

…panel content with panel.


          nit(combobox): changed double quotes to single quotes.

065adf1


          nit(combobox): changed class names and parameter types.

ddff626

nielsr98 force-pushed the combobox-open-actions branch from 15dface to ddff626 Compare

August 14, 2020 17:12

nielsr98 added the target: minor label


          fix(combobox): changed import to relative import.

4ce24aa

jelbourn added action: merge merge safe merge: fix commit message labels

andrewseguin merged commit 809f157 into angular:master

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

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

angular-automatic-lock-bot bot locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.