Cdk listbox dev app #20010

nielsr98 · 2020-07-16T15:42:48Z

Made an example for listbox in the dev-app and added selection of options via arrow key navigation while pressing shift.

…iles.

…descendant.

…ew engine bugs.

devversion · 2020-07-17T09:20:58Z

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

+    it('should focus and toggle the next item when pressing SHIFT + DOWN_ARROW', () => {
+      let selectedOptions = optionInstances.filter(option => option.selected);
+      const downKeyEvent = createKeyboardEvent('keydown', DOWN_ARROW);
+      Object.defineProperty(downKeyEvent, 'shiftKey', {get: () => true});


You should pass the modifiers directly to createKeyboardEvent (so we avoid the defineProperty call here).

devversion · 2020-07-17T09:25:03Z

src/cdk-experimental/listbox/listbox.ts

      '[attr.aria-activedescendant]': '_getAriaActiveDescendant()'
    }
 })
 export class CdkListbox implements AfterContentInit, OnDestroy, OnInit {

  _listKeyManager: ActiveDescendantKeyManager<CdkOption>;
  _selectionModel: SelectionModel<CdkOption>;
+  tabIndex = 0;


Should this be prefixed with an underscore? (similarly to how we do it within the selection list)

Yeah probably to stay consistent.

src/cdk-experimental/listbox/listbox.ts

devversion · 2020-07-17T09:32:13Z

src/dev-app/cdk-experimental-listbox/cdk-listbox-demo.html

@@ -0,0 +1,10 @@
+<ul cdkListbox class="example-listbox" [multiple]="multiSelectable" [useActiveDescendant]="activeDescendant">
+  <li class="example-option" cdkOption>Apple</li>


Is this class a noop right now?

Ah yes I forgot to remove it after I changed the way I did the styling.

devversion · 2020-07-17T09:33:24Z

src/dev-app/cdk-experimental-listbox/cdk-listbox-demo.ts

+    styleUrls: ['cdk-listbox-demo.css'],
+})
+export class CdkListboxDemo {
+    multiSelectable = false;


Nit: This file should use 2 spaces for indentation.

^{We should set up a formatter at some point again: #19879}

…o createKeyboardEvent.

jelbourn · 2020-07-20T20:33:36Z

src/dev-app/cdk-experimental-listbox/cdk-listbox-demo.css

+  padding: 0;
+}
+
+.cdk-option-selected {


Could you change these styles to only target css classes prefixed with demo-? It makes debugging the dev-app easier if you can know immediately where the styles are coming from

jelbourn · 2020-07-20T20:36:10Z

src/cdk-experimental/listbox/listbox.ts

+
+    /** Will select an option if shift was pressed while navigating to the option */
+    const isArrow = (keyCode === UP_ARROW || keyCode === DOWN_ARROW);
+    if (isArrow && event.shiftKey && previousActiveIndex !== this._listKeyManager.activeItemIndex) {


What's the thinking behind toggling options based on arrow keys with shift rather than requiring a separate space/enter keypress? Was this from the aria authoring practices?

So it would work with a separate space/enter keypress. In the authoring practices it says that selection via SHIFT + arrow key is an optional way to do selection.

devversion

LGTM for dev-app and build file changes. I'll leave the listbox shift selection change to others (looks reasonable to me though)

jelbourn

LGTM

* build: Added required files to listbox directory. * build: added listbox option directive and renamed listbox directive files. * build: Added required files to listbox directory. * build: added listbox option directive and renamed listbox directive files. * build: Added required files to listbox directory. * build: added listbox option directive and renamed listbox directive files. * build: Added required files to listbox directory. * build: added listbox option directive and renamed listbox directive files. * feat(listbox): added support for non-multiple listbox and aria activedescendant. * fix(listbox): formatted BUILD.bazel. * feat(dev-app/listbox): added cdk listbox example to the dev-app. * feat(listbox): added shift key selection. * test(listbox): added test for selection via shift key. * fix(listbox): formatted dev app build file. * fix(listbox): added codeowners for dev-app listbox example. * fix(listbox): removed width css property. * fix(listbox): removed private properties from host bindings to fix view engine bugs. * nit(listbox): changed double quote to single quote. * refactor(listbox): fixed some styling errors and added modifier key to createKeyboardEvent. * nit(listbox): changed spacing. * nit(listbox): spacing.: * nit(listbox): removed extra blank line. * refactor(listbox): changed test to have a stronger check on selected. * refactor(listbox): changed css classes to use demo- prefix. (cherry picked from commit add6ad1)

angular-automatic-lock-bot · 2020-08-21T00:08:55Z

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 requested a review from jelbourn July 16, 2020 15:43

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

nielsr98 added 13 commits July 16, 2020 12:53

build: Added required files to listbox directory.

d3fbbfc

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

5066335

…iles.

build: Added required files to listbox directory.

d0133cd

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

1e84c2d

…iles.

build: Added required files to listbox directory.

e22b3f2

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

a9b2519

…iles.

build: Added required files to listbox directory.

b1da11c

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

0af787c

…iles.

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

7ad3615

…descendant.

fix(listbox): formatted BUILD.bazel.

86510e4

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

41b62da

feat(listbox): added shift key selection.

83db6b5

test(listbox): added test for selection via shift key.

71d9806

nielsr98 force-pushed the cdk-listbox-dev-app branch from 087865d to 71d9806 Compare July 16, 2020 17:28

nielsr98 requested a review from scriby July 16, 2020 17:29

fix(listbox): formatted dev app build file.

e30d8e5

nielsr98 marked this pull request as ready for review July 16, 2020 17:32

nielsr98 requested review from devversion and mmalerba as code owners July 16, 2020 17:32

fix(listbox): added codeowners for dev-app listbox example.

169ee9c

nielsr98 requested a review from a team as a code owner July 16, 2020 17:38

nielsr98 added 3 commits July 16, 2020 13:46

fix(listbox): removed width css property.

817c6df

fix(listbox): removed private properties from host bindings to fix vi…

f7454da

…ew engine bugs.

nit(listbox): changed double quote to single quote.

127355d

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

devversion reviewed Jul 17, 2020

View reviewed changes

nielsr98 added 2 commits July 17, 2020 12:04

refactor(listbox): fixed some styling errors and added modifier key t…

72287d4

…o createKeyboardEvent.

nit(listbox): changed spacing.

1b24b51

nielsr98 added 3 commits July 17, 2020 14:35

nit(listbox): spacing.:

5278f78

nit(listbox): removed extra blank line.

d765094

refactor(listbox): changed test to have a stronger check on selected.

033f30a

jelbourn reviewed Jul 20, 2020

View reviewed changes

refactor(listbox): changed css classes to use demo- prefix.

02dc398

devversion approved these changes Jul 21, 2020

View reviewed changes

jelbourn approved these changes Jul 21, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release and removed target: minor This PR is targeted for the next minor release labels Jul 21, 2020

wagnermaciel merged commit add6ad1 into angular:master Jul 21, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Cdk listbox dev app #20010

Cdk listbox dev app #20010

Uh oh!

nielsr98 commented Jul 16, 2020 •

edited

Loading

Uh oh!

devversion Jul 17, 2020

Uh oh!

devversion Jul 17, 2020

Uh oh!

nielsr98 Jul 17, 2020

Uh oh!

Uh oh!

devversion Jul 17, 2020

Uh oh!

nielsr98 Jul 17, 2020

Uh oh!

devversion Jul 17, 2020

Uh oh!

jelbourn Jul 20, 2020

Uh oh!

jelbourn Jul 20, 2020

Uh oh!

nielsr98 Jul 20, 2020

Uh oh!

devversion left a comment

Uh oh!

jelbourn left a comment

Uh oh!

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

Uh oh!

Uh oh!

		@@ -0,0 +1,10 @@
		<ul cdkListbox class="example-listbox" [multiple]="multiSelectable" [useActiveDescendant]="activeDescendant">
		<li class="example-option" cdkOption>Apple</li>

Cdk listbox dev app #20010

Cdk listbox dev app #20010

Uh oh!

Conversation

nielsr98 commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

devversion 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 21, 2020

Uh oh!

Uh oh!

nielsr98 commented Jul 16, 2020 •

edited

Loading