Skip to content

Commit 298cdc0

Browse files
andy9775andy9775
authored
refactor(cdk-experimental/menu): Break MenuItems into directives (#19688)
* refactor(cdk-experimental/menu): Break MenuItems into directives Currently CdkMenuItem is designed to handle the logic for the radio and checkox role along with performing basic user actions and opening an attached Menu. Separating the logic into separate directives for each role makes the code easier to reason about and more testable and reliable. It also aids in implementing future functionality. * test(cdk-experimental/menu): Remove unnecessary tests MenuItemTriggers selector specifies that it must be placed alongside a cdkMenuItem directive hence placing alongside a cdkMenuItenCheckbox (or any other selectable) should be a compile time failure. * fix(cdk-experimental/menu): Fix attribute inheritance Element attributes in the host bindings are inherited by subclasses with Ivy but not View Engine. Specifying explicitly on each subclass fixes this issue. * fix(cdk-experimental/menu): Fix ViewEngine tests In ViewEngine when selecting for ContentChildren, it will include the current component if it matches the query (including subclasses). This ensures that a check is performed to prevent this bug from causing the change emitter from being completed when only a CdkMenu exists with no real nested groups. * test(cdk-experimental): Clear up tests * refactor(cdk-experimental/menu): Make internal method private * test(cdk-experimental/menu): clear up test description * refactor(cdk-experimental/menu): Rename change event emitter Change event emitter emits when the list of Selectables gets updated * test(cdk-experimental/menu): Refactor test configuration compileComponents() is an async function and therefore createComponents cannot follow it. Breaking up into seperate beforeEach blocks prevents any potential issues * refactor(cdk-experimental/menu): Rename registration method Extract logic from the lifecycle method and place into a more readable method * refactor(cdk-experimental/menu): rename native elements for clarity * refactor(cdk-experimental/menu): use the same counter for name and id * style(cdk-experimental/menu): clean up whitespace * test(cdk-experimental/menu): refactor to use for..of where prefered * style(cdk-experimental): move comment outside of conditional block * refactor(cdk-experimental/menu): move logic out of lifecycle methods * refactor(cdk-experimental/menu): simplify checkbox toggle logic * docs(cdk-experimental/menu): add clearer documentation for checkbox and radio * refactor(cdk-experimental/menu): refactor checked attribute to be null not false
1 parent b2577a1 commit 298cdc0

20 files changed

+786
-220
lines changed

‎src/cdk-experimental/menu/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ ng_module(
1111
module_name = "@angular/cdk-experimental/menu",
1212
deps = [
1313
"//src/cdk/coercion",
14+
"//src/cdk/collections",
1415
"@npm//@angular/core",
1516
"@npm//rxjs",
1617
],
@@ -24,6 +25,7 @@ ng_test_library(
2425
),
2526
deps = [
2627
":menu",
28+
"//src/cdk/collections",
2729
"@npm//@angular/platform-browser",
2830
],
2931
)

‎src/cdk-experimental/menu/menu-bar.spec.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,14 @@
11
import {ComponentFixture, TestBed, async} from '@angular/core/testing';
22
import {Component} from '@angular/core';
3+
import {By} from '@angular/platform-browser';
34
import {CdkMenuBar} from './menu-bar';
45
import {CdkMenuModule} from './menu-module';
5-
import {CdkMenuItem} from './menu-item';
6-
import {By} from '@angular/platform-browser';
6+
import {CdkMenuItemRadio} from './menu-item-radio';
77

88
describe('MenuBar', () => {
99
describe('as radio group', () => {
1010
let fixture: ComponentFixture<MenuBarRadioGroup>;
11-
let menuItems: CdkMenuItem[];
11+
let menuItems: CdkMenuItemRadio[];
1212

1313
beforeEach(async(() => {
1414
TestBed.configureTestingModule({
@@ -20,8 +20,8 @@ describe('MenuBar', () => {
2020
fixture.detectChanges();
2121

2222
menuItems = fixture.debugElement
23-
.queryAll(By.directive(CdkMenuItem))
24-
.map(element => element.injector.get(CdkMenuItem));
23+
.queryAll(By.directive(CdkMenuItemRadio))
24+
.map(element => element.injector.get(CdkMenuItemRadio));
2525
}));
2626

2727
it('should toggle menuitemradio items', () => {
@@ -37,7 +37,7 @@ describe('MenuBar', () => {
3737

3838
describe('radiogroup change events', () => {
3939
let fixture: ComponentFixture<MenuBarRadioGroup>;
40-
let menuItems: CdkMenuItem[];
40+
let menuItems: CdkMenuItemRadio[];
4141

4242
beforeEach(async(() => {
4343
TestBed.configureTestingModule({
@@ -50,8 +50,8 @@ describe('MenuBar', () => {
5050
fixture.detectChanges();
5151

5252
menuItems = fixture.debugElement
53-
.queryAll(By.directive(CdkMenuItem))
54-
.map(element => element.injector.get(CdkMenuItem));
53+
.queryAll(By.directive(CdkMenuItemRadio))
54+
.map(element => element.injector.get(CdkMenuItemRadio));
5555
}));
5656

5757
it('should emit on click', () => {
@@ -73,12 +73,12 @@ describe('MenuBar', () => {
7373
template: `
7474
<ul cdkMenuBar>
7575
<li role="none">
76-
<button checked="true" role="menuitemradio" cdkMenuItem>
76+
<button checked="true" cdkMenuItemRadio>
7777
first
7878
</button>
7979
</li>
8080
<li role="none">
81-
<button role="menuitemradio" cdkMenuItem>
81+
<button cdkMenuItemRadio>
8282
second
8383
</button>
8484
</li>

‎src/cdk-experimental/menu/menu-bar.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import {CdkMenuGroup} from './menu-group';
1111

1212
/**
1313
* Directive applied to an element which configures it as a MenuBar by setting the appropriate
14-
* role, aria attributes, and accessable keyboard and mouse handling logic. The component that
14+
* role, aria attributes, and accessible keyboard and mouse handling logic. The component that
1515
* this directive is applied to should contain components marked with CdkMenuItem.
1616
*
1717
*/

‎src/cdk-experimental/menu/menu-group.spec.ts

Lines changed: 23 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,15 @@
11
import {Component} from '@angular/core';
22
import {ComponentFixture, TestBed, async} from '@angular/core/testing';
3+
import {By} from '@angular/platform-browser';
34
import {CdkMenuModule} from './menu-module';
45
import {CdkMenuGroup} from './menu-group';
5-
import {CdkMenuItem} from './menu-item';
6-
import {CdkMenu} from './menu';
7-
import {By} from '@angular/platform-browser';
6+
import {CdkMenuItemCheckbox} from './menu-item-checkbox';
7+
import {CdkMenuItemRadio} from './menu-item-radio';
88

99
describe('MenuGroup', () => {
1010
describe('with MenuItems as checkbox', () => {
1111
let fixture: ComponentFixture<CheckboxMenu>;
12-
let menuItems: CdkMenuItem[];
12+
let menuItems: CdkMenuItemCheckbox[];
1313

1414
beforeEach(async(() => {
1515
TestBed.configureTestingModule({
@@ -21,8 +21,8 @@ describe('MenuGroup', () => {
2121
fixture.detectChanges();
2222

2323
menuItems = fixture.debugElement
24-
.queryAll(By.directive(CdkMenuItem))
25-
.map(e => e.injector.get(CdkMenuItem));
24+
.queryAll(By.directive(CdkMenuItemCheckbox))
25+
.map(e => e.injector.get(CdkMenuItemCheckbox));
2626
}));
2727

2828
it('should not change state of sibling checked menuitemcheckbox', () => {
@@ -34,7 +34,7 @@ describe('MenuGroup', () => {
3434

3535
describe('with MenuItems as radio button', () => {
3636
let fixture: ComponentFixture<MenuWithMultipleRadioGroups>;
37-
let menuItems: CdkMenuItem[];
37+
let menuItems: CdkMenuItemRadio[];
3838

3939
beforeEach(async(() => {
4040
TestBed.configureTestingModule({
@@ -46,8 +46,8 @@ describe('MenuGroup', () => {
4646
fixture.detectChanges();
4747

4848
menuItems = fixture.debugElement
49-
.queryAll(By.directive(CdkMenuItem))
50-
.map(e => e.injector.get(CdkMenuItem));
49+
.queryAll(By.directive(CdkMenuItemRadio))
50+
.map(e => e.injector.get(CdkMenuItemRadio));
5151
}));
5252

5353
it('should change state of sibling menuitemradio in same group', () => {
@@ -76,8 +76,7 @@ describe('MenuGroup', () => {
7676

7777
describe('change events', () => {
7878
let fixture: ComponentFixture<MenuWithMenuItemsAndRadioGroups>;
79-
let menu: CdkMenu;
80-
let menuItems: CdkMenuItem[];
79+
let menuItems: CdkMenuItemRadio[];
8180

8281
beforeEach(async(() => {
8382
TestBed.configureTestingModule({
@@ -88,22 +87,11 @@ describe('MenuGroup', () => {
8887
fixture = TestBed.createComponent(MenuWithMenuItemsAndRadioGroups);
8988
fixture.detectChanges();
9089

91-
menu = fixture.debugElement.query(By.directive(CdkMenu)).injector.get(CdkMenu);
92-
9390
menuItems = fixture.debugElement
94-
.queryAll(By.directive(CdkMenuItem))
95-
.map(element => element.injector.get(CdkMenuItem));
91+
.queryAll(By.directive(CdkMenuItemRadio))
92+
.map(element => element.injector.get(CdkMenuItemRadio));
9693
}));
9794

98-
it('should not emit from root menu with nested groups', () => {
99-
const spy = jasmine.createSpy('changeSpy for root menu');
100-
menu.change.subscribe(spy);
101-
102-
menuItems.forEach(menuItem => menuItem.trigger());
103-
104-
expect(spy).toHaveBeenCalledTimes(0);
105-
});
106-
10795
it('should emit from enclosing radio group only', () => {
10896
const spies: jasmine.Spy[] = [];
10997

@@ -117,8 +105,8 @@ describe('MenuGroup', () => {
117105

118106
expect(spies[1]).toHaveBeenCalledTimes(1);
119107
expect(spies[1]).toHaveBeenCalledWith(menuItems[0]);
120-
expect(spies[2]).toHaveBeenCalledTimes(0);
121-
expect(spies[3]).toHaveBeenCalledTimes(0);
108+
expect(spies[2]).not.toHaveBeenCalled();
109+
expect(spies[3]).not.toHaveBeenCalled();
122110
});
123111

124112
it('should not emit with click on disabled button', () => {
@@ -132,21 +120,7 @@ describe('MenuGroup', () => {
132120

133121
menuItems[0].trigger();
134122

135-
expect(spy).toHaveBeenCalledTimes(0);
136-
});
137-
138-
it('should not emit on menuitem click', () => {
139-
const spies: jasmine.Spy[] = [];
140-
141-
fixture.debugElement.queryAll(By.directive(CdkMenuGroup)).forEach((group, index) => {
142-
const spy = jasmine.createSpy(`cdkMenuGroup ${index} change spy`);
143-
spies.push(spy);
144-
group.injector.get(CdkMenuGroup).change.subscribe(spy);
145-
});
146-
147-
menuItems[2].trigger();
148-
149-
spies.forEach(spy => expect(spy).toHaveBeenCalledTimes(0));
123+
expect(spy).not.toHaveBeenCalled();
150124
});
151125
});
152126
});
@@ -157,12 +131,12 @@ describe('MenuGroup', () => {
157131
<li role="none">
158132
<ul cdkMenuGroup>
159133
<li #first role="none">
160-
<button checked="true" role="menuitemcheckbox" cdkMenuItem>
134+
<button checked="true" cdkMenuItemCheckbox>
161135
one
162136
</button>
163137
</li>
164138
<li role="none">
165-
<button role="menuitemcheckbox" cdkMenuItem>
139+
<button cdkMenuItemCheckbox>
166140
two
167141
</button>
168142
</li>
@@ -179,12 +153,12 @@ class CheckboxMenu {}
179153
<li role="none">
180154
<ul cdkMenuGroup>
181155
<li role="none">
182-
<button checked="true" role="menuitemradio" cdkMenuItem>
156+
<button checked="true" cdkMenuItemRadio>
183157
one
184158
</button>
185159
</li>
186160
<li role="none">
187-
<button role="menuitemradio" cdkMenuItem>
161+
<button cdkMenuItemRadio>
188162
two
189163
</button>
190164
</li>
@@ -193,12 +167,12 @@ class CheckboxMenu {}
193167
<li role="none">
194168
<ul cdkMenuGroup>
195169
<li role="none">
196-
<button role="menuitemradio" cdkMenuItem>
170+
<button cdkMenuItemRadio>
197171
three
198172
</button>
199173
</li>
200174
<li role="none">
201-
<button role="menuitemradio" cdkMenuItem>
175+
<button cdkMenuItemRadio>
202176
four
203177
</button>
204178
</li>
@@ -215,7 +189,7 @@ class MenuWithMultipleRadioGroups {}
215189
<li role="none">
216190
<ul cdkMenuGroup>
217191
<li role="none">
218-
<button role="menuitemradio" cdkMenuItem>
192+
<button cdkMenuItemRadio>
219193
one
220194
</button>
221195
</li>
@@ -224,7 +198,7 @@ class MenuWithMultipleRadioGroups {}
224198
<li role="none">
225199
<ul cdkMenuGroup>
226200
<li role="none">
227-
<button role="menuitemradio" cdkMenuItem>
201+
<button cdkMenuItemRadio>
228202
two
229203
</button>
230204
</li>

‎src/cdk-experimental/menu/menu-group.ts

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,19 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {Directive, Output, EventEmitter} from '@angular/core';
10-
import {MenuItem} from './menu-item-interface';
9+
import {
10+
Directive,
11+
Output,
12+
EventEmitter,
13+
ContentChildren,
14+
AfterContentInit,
15+
QueryList,
16+
OnDestroy,
17+
} from '@angular/core';
18+
import {UniqueSelectionDispatcher} from '@angular/cdk/collections';
19+
import {takeUntil} from 'rxjs/operators';
20+
import {CdkMenuItemSelectable} from './menu-item-selectable';
21+
import {CdkMenuItem} from './menu-item';
1122

1223
/**
1324
* Directive which acts as a grouping container for `CdkMenuItem` instances with
@@ -19,18 +30,45 @@ import {MenuItem} from './menu-item-interface';
1930
host: {
2031
'role': 'group',
2132
},
33+
providers: [{provide: UniqueSelectionDispatcher, useClass: UniqueSelectionDispatcher}],
2234
})
23-
export class CdkMenuGroup {
35+
export class CdkMenuGroup implements AfterContentInit, OnDestroy {
2436
/** Emits the element when checkbox or radiobutton state changed */
25-
@Output() change: EventEmitter<MenuItem> = new EventEmitter();
37+
@Output() readonly change: EventEmitter<CdkMenuItem> = new EventEmitter();
38+
39+
/** List of menuitemcheckbox or menuitemradio elements which reside in this group */
40+
@ContentChildren(CdkMenuItemSelectable, {descendants: true})
41+
private readonly _selectableItems: QueryList<CdkMenuItemSelectable>;
42+
43+
/** Emits when the _selectableItems QueryList triggers a change */
44+
private readonly _selectableChanges: EventEmitter<void> = new EventEmitter();
45+
46+
ngAfterContentInit() {
47+
this._registerMenuSelectionListeners();
48+
}
2649

2750
/**
28-
* Emits events for the clicked MenuItem
29-
* @param menuItem The clicked MenuItem to handle
51+
* Register the child selectable elements with the change emitter and ensure any new child
52+
* elements do so as well.
3053
*/
31-
_registerTriggeredItem(menuItem: MenuItem) {
32-
if (menuItem.role !== 'menuitem') {
33-
this.change.emit(menuItem);
34-
}
54+
private _registerMenuSelectionListeners() {
55+
this._selectableItems.forEach(selectable => this._registerClickListener(selectable));
56+
57+
this._selectableItems.changes.subscribe((selectableItems: QueryList<CdkMenuItemSelectable>) => {
58+
this._selectableChanges.next();
59+
selectableItems.forEach(selectable => this._registerClickListener(selectable));
60+
});
61+
}
62+
63+
/** Register each selectable to emit on the change Emitter when clicked */
64+
private _registerClickListener(selectable: CdkMenuItemSelectable) {
65+
selectable.clicked
66+
.pipe(takeUntil(this._selectableChanges))
67+
.subscribe(() => this.change.next(selectable));
68+
}
69+
70+
ngOnDestroy() {
71+
this._selectableChanges.next();
72+
this._selectableChanges.complete();
3573
}
3674
}

0 commit comments

Comments
 (0)