Skip to content
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

feat(cdk-experimental/listbox): selection logic and testing for listbox. #19690

Merged
merged 35 commits into from
Jun 26, 2020
Merged
Show file tree
Hide file tree
Changes from 28 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8ff6997
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
364aa53
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
ee5746b
build: Added required files to listbox directory.
nielsr98 Jun 11, 2020
893ba4f
build: added listbox option directive and renamed listbox directive f…
nielsr98 Jun 11, 2020
5d0617c
fix(cdk-experimental/listbox): deleted unused files.
nielsr98 Jun 18, 2020
a3a6940
feat(cdk-experimental/listbox): added tests for listbox and options, …
nielsr98 Jun 18, 2020
24acc81
chore(cdk-experimental/listbox): formated BUILD.bazel.
nielsr98 Jun 18, 2020
a0d66ad
fix(cdk-experimental/listbox): fixed style bugs caught by lint test.
nielsr98 Jun 18, 2020
9369e30
chore(cdk-experimental/listbox): fixed style bugs and made CdkListbox…
nielsr98 Jun 18, 2020
b5ba387
fix(cdk-experimental/listbox): made click handler function public.
nielsr98 Jun 18, 2020
2b4b436
refactor(cdk-experimental/listbox): refactored functions to condense …
nielsr98 Jun 18, 2020
362c169
fix(cdk-experimental/listbox): removed unused import.
nielsr98 Jun 18, 2020
4edff93
refactor(cdk-experimental/listbox): cleaned up variable and method na…
nielsr98 Jun 22, 2020
e13c411
:
nielsr98 Jun 22, 2020
02a0d59
fix(cdk-experimental/listbox): moved compileComponents and createComp…
nielsr98 Jun 22, 2020
41bda71
refactor(cdk-experimental/listbox): used DebugElement for testing ins…
nielsr98 Jun 22, 2020
553df4c
refactor(cdk-experimental/listbox): removed unused variable.
nielsr98 Jun 22, 2020
a66964e
fix(cdk-experimental/listbox): removed unused import.
nielsr98 Jun 22, 2020
8aac577
feat(cdk-experimental/listbox): created selectionChange event emitter…
nielsr98 Jun 25, 2020
4f9666f
refactor(cdk-experimental/listbox): removed click handler on listbox …
nielsr98 Jun 25, 2020
c375efa
refactor(cdk-experimental/listbox): created selectionChange emitter f…
nielsr98 Jun 25, 2020
0060b40
fix(cdk-experimental:listbox): added toggle function to change select…
nielsr98 Jun 25, 2020
5332e83
refactor(cdk-experimental/listbox): added tests for unique ids and th…
nielsr98 Jun 25, 2020
93a9d44
refactor(cdk-experimental/listbox): removed unused function to get el…
nielsr98 Jun 25, 2020
5df2643
fix(cdk-experimental/listbox): fixed double quote lint error.
nielsr98 Jun 26, 2020
76e4232
refactor(cdk-experimental/listbox): coerce boolean property when sett…
nielsr98 Jun 26, 2020
b900539
fix(cdk-experimental/listbox): add static ngAcceptInputType for _sele…
nielsr98 Jun 26, 2020
fa2e1da
fix(cdk-experimental/listbox): fixed BUILD file format issue.
nielsr98 Jun 26, 2020
618d9c6
refactor(cdk-experimental/listbox): use the id attribute instead of c…
nielsr98 Jun 26, 2020
48e792f
fix(cdk-experimental/listbox): removed unused variable.
nielsr98 Jun 26, 2020
d39d9ae
fix(cdk-experimental/listbox): removed unused listbox variable.
nielsr98 Jun 26, 2020
75de55c
fix(cdk-experimental/listbox): removed unused CdkListbox import.
nielsr98 Jun 26, 2020
1e5f5ee
feat(cdk-experimental/listbox): added interface for selection change …
nielsr98 Jun 26, 2020
5be6915
fix(cdk-experimental/listbox): fix line length.
nielsr98 Jun 26, 2020
a0eaea2
refactor(cdk-experimental/listbox): removed spy from test.
nielsr98 Jun 26, 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
23 changes: 22 additions & 1 deletion src/cdk-experimental/listbox/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
load("//tools:defaults.bzl", "ng_module")
load("//tools:defaults.bzl", "ng_module", "ng_test_library", "ng_web_test_suite")

package(default_visibility = ["//visibility:public"])

Expand All @@ -9,4 +9,25 @@ ng_module(
exclude = ["**/*.spec.ts"],
),
module_name = "@angular/cdk-experimental/listbox",
deps = [
"//src/cdk/coercion",
],
)

ng_test_library(
name = "unit_test_sources",
srcs = glob(
["**/*.spec.ts"],
exclude = ["**/*.e2e.spec.ts"],
),
deps = [
":listbox",
"//src/cdk/testing/private",
"@npm//@angular/platform-browser",
],
)

ng_web_test_suite(
name = "unit_tests",
deps = [":unit_test_sources"],
)
102 changes: 102 additions & 0 deletions src/cdk-experimental/listbox/listbox.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
import {
ComponentFixture,
async,
TestBed,
} from '@angular/core/testing';
import {Component, DebugElement} from '@angular/core';
import {By} from '@angular/platform-browser';
import {
CdkOption,
CdkListboxModule, CdkListbox
} from './index';
import {dispatchMouseEvent} from '@angular/cdk/testing/private';

describe('CdkOption', () => {

describe('selection state change', () => {
let fixture: ComponentFixture<ListboxWithCdkOptions>;
let listbox: DebugElement;
let listboxInstance: CdkListbox;
let options: DebugElement[];
let optionInstances: CdkOption[];
let optionElements: Element[];
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
let optionElements: Element[];
let optionElements: HTMLElement[];

You can typically be a little bit more specific with these and specify HTMLElement


beforeEach(async(() => {
TestBed.configureTestingModule({
imports: [CdkListboxModule],
declarations: [ListboxWithCdkOptions],
}).compileComponents();
}));

beforeEach(async(() => {
fixture = TestBed.createComponent(ListboxWithCdkOptions);
fixture.detectChanges();

listbox = fixture.debugElement.query(By.directive(CdkListbox));
listboxInstance = listbox.injector.get<CdkListbox>(CdkListbox);

options = fixture.debugElement.queryAll(By.directive(CdkOption));
jelbourn marked this conversation as resolved.
Show resolved Hide resolved
optionInstances = options.map(o => o.injector.get<CdkOption>(CdkOption));
optionElements = options.map(o => o.nativeElement);
}));

it('should generate a unique optionId for each option', () => {
let optionIds: string[] = [];
for (const instance of optionInstances) {
const id = instance._optionid;

expect(optionIds.indexOf(id)).toBe(-1);
optionIds.push(id);

expect(id).toMatch(/cdk-option-\d+/);
}
});

it('should have set the selected input of the options to null by default', () => {
for (const instance of optionInstances) {
expect(instance.selected).toBeFalse();
}
});

it('should update aria-selected when selected is changed programmatically', () => {
expect(optionElements[0].getAttribute('aria-selected')).toBeNull();
optionInstances[1].selected = true;
fixture.detectChanges();

expect(optionElements[1].getAttribute('aria-selected')).toBe('true');
});

it('should update selected option on click event', () => {
jelbourn marked this conversation as resolved.
Show resolved Hide resolved
let selectedOptions = optionInstances.filter(option => option.selected);
spyOn(listboxInstance, '_emitChangeEvent');
Copy link
Member

Choose a reason for hiding this comment

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

Rather than setting up a spy on the internal listbox method, you can have your ListboxWithCdkOptions component below add a (selectionChange) handler to the listbox and assert that that's called. Generally you want to avoid having the test know about the internals of the component unless there's no other practical way to test something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I see. Will change.


expect(selectedOptions.length).toBe(0);
expect(optionElements[0].getAttribute('aria-selected')).toBeNull();
expect(optionInstances[0].selected).toBeFalse();
expect(listboxInstance._emitChangeEvent).toHaveBeenCalledTimes(0);

dispatchMouseEvent(optionElements[0], 'click');
fixture.detectChanges();

selectedOptions = optionInstances.filter(option => option.selected);
expect(selectedOptions.length).toBe(1);
expect(optionElements[0].getAttribute('aria-selected')).toBe('true');
expect(optionInstances[0].selected).toBeTrue();
expect(listboxInstance._emitChangeEvent).toHaveBeenCalledTimes(1);
});
});

});

@Component({
template: `
<div cdkListbox>
<div cdkOption>Void</div>
<div cdkOption>Solar</div>
<div cdkOption>Arc</div>
<div cdkOption>Stasis</div>
</div>`
})
class ListboxWithCdkOptions {
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
class ListboxWithCdkOptions {
class ListboxWithOptions {

can also omit Cdk form the option part


}
64 changes: 63 additions & 1 deletion src/cdk-experimental/listbox/listbox.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,62 @@
* found in the LICENSE file at https://angular.io/license
*/

import {Directive} from '@angular/core';
import {
ContentChildren,
Directive,
EventEmitter, forwardRef, Inject,
Input, Output,
QueryList
} from '@angular/core';
import {BooleanInput, coerceBooleanProperty} from '@angular/cdk/coercion';

/**
* Directive that applies interaction patterns to an element following the aria role of option.
* Typically meant to be placed inside a listbox. Logic handling selection, disabled state, and
* value is built in.
*/
@Directive({
selector: '[cdkOption]',
exportAs: 'cdkOption',
host: {
role: 'option',
'(click)': 'toggle()',
'[attr.aria-selected]': '_selected || null',
'[attr.optionid]': '_optionid',
Copy link
Member

Choose a reason for hiding this comment

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

It does need to be the native id property since aria-activedescendant relies on id. You can look at MatCheckbox for an example how we do do this, but by making id an @Input and assigning it an initial value, you ensure that it always has an ID while also letting the user set one if they need to for some reason (but people generally don't want to, that's why they're using a common component).

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 see I think select does that as well. I'll fix this.

}
})
export class CdkOption {
private _selected: boolean = false;
readonly _optionid: string;

/** Whether the option is selected or not */
@Input()
get selected(): boolean {
return this._selected;
}
set selected(value: boolean) {
this._selected = coerceBooleanProperty(value);
}

constructor(@Inject(forwardRef(() => CdkListbox)) public listbox: CdkListbox) {
this._optionid = `cdk-option-${nextId++}`;
}

toggle() {
this.selected = !this.selected;
this.listbox._emitChangeEvent(this);
}

static ngAcceptInputType_selected: BooleanInput;
}

let nextId = 0;
Copy link
Member

Choose a reason for hiding this comment

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

I would typically put this above the class where it's used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah true I used to use it in Listbox and neglected moving it.


/**
* Directive that applies interaction patterns to an element following the aria role of listbox.
* Typically CdkOption elements are placed inside the listbox. Logic to handle keyboard navigation,
* selection of options, active options, and disabled states is built in.
*/
@Directive({
selector: '[cdkListbox]',
exportAs: 'cdkListbox',
Expand All @@ -28,4 +71,23 @@ export class CdkOption {
})
export class CdkListbox {

/** A query list containing all CdkOption elements within this listbox */
@ContentChildren(CdkOption, {descendants: true}) _options: QueryList<CdkOption>;

@Output() readonly selectionChange: EventEmitter<CdkOption> = new EventEmitter<CdkOption>();
Copy link
Member

Choose a reason for hiding this comment

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

I forgot if I mentioned this earlier, but in a follow-up we'll want to change this from emitting CdkOption to emitting an event interface (something like ListboxSelectionChangeEvent)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright sounds good.

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 looked into it and it seemed pretty straightforward so I rolled it into a commit in this PR. If I didn't implement it right or you think it should go in separately I can remove it.


/** Emits a selection change event, called when an option has its selected state changed */
_emitChangeEvent(option: CdkOption) {
this.selectionChange.emit(option);
}

/** Sets the given option's selected state to true */
select(option: CdkOption) {
option.selected = true;
}

/** Sets the given option's selected state to null. Null is preferable for screen readers */
deselect(option: CdkOption) {
option.selected = false;
}
}