feat(cdk-experimental/menu): Implement grouping logic for menuitems #19628

andy9775 · 2020-06-12T18:52:36Z

The MenuBar and Menu pattern allows for MenuItems which have the
menuitemcheckbox or menuitemradio roles. This implements the grouping
logic for, menuitemradio which ensures that sibling items within a
CdkMenu or CdkMenGroup are logically connected and only one can be
checked. It also implements the logic for menuitemcheckbox. Further it
implements the logic for emitting checked events for elements marked
menuitemcheckbox and menuitemradio from their enclosing groups.

Note that I opted not to allow the disabled property on CdkMenu and CdkMenuGroup since this follows more in line with mat-menu, and how I interpreted the pattern. However if we decide to implement this functionality it can be easily added with a followup pr.

The MenuBar and Menu pattern allows for MenuItems which have the menuitemcheckbox or menuitemradio roles. This implements the grouping logic for, menuitemradio which ensures that sibling items within a CdkMenu or CdkMenGroup are logically connected and only one can be checked. It also implements the logic for menuitemcheckbox. Further it implements the logic for emitting checked events for elements marked menuitemcheckbox and menuitemradio from their enclosing groups.

teflonwaffles · 2020-06-12T19:08:32Z

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

+      expect(menuItems[0].checked).toBeTrue();
+      expect(menuItems[1].checked).toBeFalse();
+
+      menuItems[1].trigger();


I'm not familiar with the public Angular testing conventions. Would this be the appropriate point to bring in test harnesses? (Would they apply for CDK directives, as opposed to actual components?)

Usually, it would be better to simulate real clicks using harness actions or mouse events, instead of directly calling methods on the directive class. But I don't know if that applies here, or if it's possible at this point in development.

We actually don't use harnesses for the component's unit test; we create separate tests for the harnesses once we make them

teflonwaffles · 2020-06-12T19:12:01Z

src/cdk-experimental/menu/menu.ts

-  @Output() change: EventEmitter<CdkMenuItem>;
+  /** All the child MenuItem components which this directive wraps including descendants */
+  @ContentChildren(CdkMenuItem, {descendants: true})
+  readonly _allItems: QueryList<CdkMenuItem>;


Typically, we'd mark the field as '_allItems!: QueryList<...>', and assert the field is not undefined in ngAfterContentInit(). But I don't know if that's the convention in the CDK.

Yeah, we don't use that convention yet because we haven't enabled --strictPropertyInitialization w/ TypeScript. We will likely do this someday, but don't have the assertion system set up presently.

teflonwaffles · 2020-06-12T19:38:13Z

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

+      menuItems = menuBar._allItems.toArray();
+    }));
+
+    it('should toggle menuitemcheckbox', () => {


If you remove words like 'should' from your test descriptions, you can make them more concise. (We can assume that all tests describe the intended behavior for the components under tests.)

Also, this description could be more specific. You're testing that menuitemcheckboxes are toggled in isolation, so if the description mentions this, you can avoid the comment further down.

This is a convention for Angular projects; we want test description to read like a sentence ("it should...")

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

teflonwaffles · 2020-06-12T19:55:48Z

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

  /**
   * Sets the aria-orientation attribute and determines where sub-menus will be opened.
   * Does not affect styling/layout.
   */
  @Input('cdkMenuBarOrientation') orientation: 'horizontal' | 'vertical' = 'horizontal';
+
+  /** All the child MenuItem components which this directive wraps including descendants */


Are you only using this to access items in the unit test? If so, it may be better to leave it out and find another way to grab the menu items for the test (using selectors, for instance). Although it's good to design things for testability, it's not a good idea to have test-only APIs. In general, they encourage testing in ways that don't match the real-world use of the components. (At the very least, you also want to reduce the size of the Angular payload by not including test-only code.)

If you do plan to use this within this component itself, then never mind (although I still wouldn't use it in tests, since it's been marked private with an underscore).

It'll be used in future functionality. I added it at this point since I can also use it in the test. But ya, I could have also added the ContentChildren into the test fixture and gone about it that way.

As for the package private - the Angular recommendation is _underscore is used internally - kind of like package private in java. @jelbourn thoughts?

We do use the underscore prefix as "library-internal". For testing, though, we we typically grab content child items directly (via debugElement.query) in the tests rather than going through the container component's ContentChildren property since it is more of an implementation detail.

As a rule of thumb, I would only add stuff to the code as it's needed for some behavior/feature.

teflonwaffles · 2020-06-12T20:00:51Z

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

@@ -0,0 +1,175 @@
+import {ComponentFixture, TestBed, async} from '@angular/core/testing';


I just realized you test a lot of the same functionality in the tests for menu-group. Since menu-bar extends menu-group, I don't think you need to repeat the same tests here -- this suite can focus on tests specific to menu-bar's extra functionality.

The menu and menu-bar tests are more of a sanity check to ensure that it's wired up correctly The meat of the logic is in the menu-group which contains most of the tests.

In that case, just one or two sanity-check tests should be enough -- I don't think you need to go over checkboxes and radio buttons separately. (That said, I initially thought there were more tests in this file, for some reason. Now that I look again, there doesn't seem to be as much overlap. I'm still not used to reviewing on Github...)

teflonwaffles · 2020-06-12T20:05:01Z

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

+    </ul>
+  `,
+})
+class MenuGroups {}


I think the smaller, focused test components from the menu-bar test suite would be better. (That way, it's easier to see the purpose of each test component, without the need for explanatory HTML comments.)

ya so the menu/menu-bar tests I made small however here I also wanted to test interaction with groups next to each other hence why it's larger than normal.

Oh, you were testing radio group independence? That's a good thing to test.

I still might consider splitting the checkbox group into a separate group, though, unless you need it in a test with radio groups.

Exactly. When I was refactoring the circular reference issue it ended up catching a potential bug so it was useful already.

I would name this component something like MenuWithMultipleSelectionGroups. Ideally you would only use this component for tests that deal specifically with interactions related to having multiple selection-toggle controls. For more basic functionality (like change events), we could create a smaller test component

src/cdk-experimental/menu/menu-item.ts

teflonwaffles · 2020-06-12T20:12:25Z

src/cdk-experimental/menu/menu-item.ts

+  /** Configure event subscriptions */
+  ngAfterContentInit() {
+    if (this.role !== 'menuitem') {
+      this._menuGroup.change.subscribe((button: CdkMenuItem) => this._toggleCheckedState(button));


Typically, we'd make sure to unsubscribe any subscriptions in ngOnDestroy, using a ReplaySubject and takeUntil (https://ultimatecourses.com/blog/exploring-angular-lifecycle-hooks-ondestroy). I don't know if there's a different convention for this in the CDK, though.

My approach here is that the emitter comes from the parent and the child wouldn't get destroyed independent of the parent hence it should auto-unsubscribe when the parent gets destroyed. I've seen it in a few places in the cdk with that assumption IIRC.

That should be fine, although it's worth considering the lazy-loaded scenario, where menu items are generated on demand. If previous menu items are destroyed (such as with a long, scrollable menu list), their subscriptions will have to be unsubscribed manually.

That said, that probably won't be in place for a while, so it should be okay to follow the convention for now.

It's actually pretty common to use ngIf or ngFor with menu items, which will destroy the items independently from the parent. You can just do the right thing here by using takeUntil(this._destroyed) with the subscription (you can see a bunch of examples of this throughout the library)

src/cdk-experimental/menu/menu-item.ts

teflonwaffles · 2020-06-12T20:15:09Z

src/cdk-experimental/menu/menu-item.ts

+  private _toggleCheckedState(selected: CdkMenuItem) {
+    if (this.role === 'menuitemradio') {
+      this.checked = selected === this;
+    } else if (this.role === 'menuitemcheckbox' && selected === this) {


I wonder if it would be a good idea to save 'menuitemradio' and 'menuitemcheckbox' as constants somewhere, so any typos become evident at compile time (instead of runtime).

These would actually be enforced by the type system already because they're string literal types

teflonwaffles · 2020-06-12T20:16:18Z

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

+      menuItems = menu._allItems.toArray();
+    }));
+
+    it('should toggle menuitemradio items', () => {


Again, I don't know if you need to replicate the menu-group tests here.

Not all MenuItems will open a submenu so this updates the reference to being optional.

Use MenuItem to help fix the circular dependency between CdkMenuItem and CdkMenuGroup. The interface is also the return type the change event handler and therefore only specifies relevant methods/properties

…group

…t fixtures

teflonwaffles · 2020-06-12T23:02:42Z

src/cdk-experimental/menu/menu-item.ts

+    if (this.hasSubmenu()) {
+      // TODO(andy): open the menu
+    } else if (this.role !== 'menuitem') {
+      this._menuGroup.change.next(this);


This is quite a clever mechanism -- I like it. (I wouldn't have thought of emitting menu item instances through an event emitter that way.)

That said, it feels a little awkward to reach into the CdkMenuGroup and call a method directly on one of its members (even if it's a public member). I might instead have a 'registerTriggeredMenuItem()' method on the menu group class -- calling it with a menu item causes the item to be emitted by (change).

Then again, since this is all package-private interaction, it may be okay this way. I'll leave it to your judgment.

Good Idea. I took it a step further and moved the logic into CdkMenuGroup - I think it makes more sense for it to determine when to emit a change event than having the MenuItem doing so.

CdkMenuGroup should determine when to emit change events on its change emitter and not the MenuItem

… and MenuGroup

…tests

jelbourn · 2020-06-15T19:52:20Z

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

  /**
   * Sets the aria-orientation attribute and determines where sub-menus will be opened.
   * Does not affect styling/layout.
   */
  @Input('cdkMenuBarOrientation') orientation: 'horizontal' | 'vertical' = 'horizontal';
+
+  /** All the child MenuItem components which this directive wraps including descendants */


We do use the underscore prefix as "library-internal". For testing, though, we we typically grab content child items directly (via debugElement.query) in the tests rather than going through the container component's ContentChildren property since it is more of an implementation detail.

As a rule of thumb, I would only add stuff to the code as it's needed for some behavior/feature.

jelbourn · 2020-06-15T19:53:14Z

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

+describe('MenuGroup', () => {
+  describe('MenuItem', () => {
+    let fixture: ComponentFixture<MenuGroups>;
+    let menuItems: Array<CdkMenuItem>;


Prefer the more concise array type syntax, CdkMenuItem[] except in cases where the type is especially complex (here and elsewhere)

jelbourn · 2020-06-15T19:56:27Z

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

+
+      menuItems = fixture.debugElement
+        .queryAll(By.directive(CdkMenuItem))
+        .map((element) => element.injector.get(CdkMenuItem));


You can omit the parentheses around element here.

Also, FYI, for simple map/filter/etc expressions like this, feel free to use single-character param names, e.g.

menuItems = fixture.debugElement .queryAll(By.directive(CdkMenuItem)) .map(e => e.injector.get(CdkMenuItem));

(though writing out element is also totally fine)

jelbourn · 2020-06-15T19:58:16Z

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

+    it('should not change state of sibling menuitemcheckbox', () => {
+      menuItems[1].trigger();
+
+      expect(menuItems[0].checked).toBeTrue(); // default state true


FYI you can add descriptions for assertions with jasmine like

expect(menuItems[0].checked).toBeTrue('Expected the first menu item to remain checked');

This makes it so that, when the test fails, you get a more descriptive failure message than "Expected false to be true". This is optional, but personally I like to add it for any boolean-based assertions.

toBeTrue does not take any arguments.

jelbourn · 2020-06-15T21:07:18Z

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

+    </ul>
+  `,
+})
+class MenuGroups {}


I would name this component something like MenuWithMultipleSelectionGroups. Ideally you would only use this component for tests that deal specifically with interactions related to having multiple selection-toggle controls. For more basic functionality (like change events), we could create a smaller test component

jelbourn · 2020-06-15T21:13:25Z

src/cdk-experimental/menu/menu-item.ts

+  /** Configure event subscriptions */
+  ngAfterContentInit() {
+    if (this.role !== 'menuitem') {
+      this._menuGroup.change.subscribe((button: CdkMenuItem) => this._toggleCheckedState(button));


It's actually pretty common to use ngIf or ngFor with menu items, which will destroy the items independently from the parent. You can just do the right thing here by using takeUntil(this._destroyed) with the subscription (you can see a bunch of examples of this throughout the library)

jelbourn · 2020-06-15T21:15:43Z

src/cdk-experimental/menu/menu-item.ts

+  private _toggleCheckedState(selected: CdkMenuItem) {
+    if (this.role === 'menuitemradio') {
+      this.checked = selected === this;
+    } else if (this.role === 'menuitemcheckbox' && selected === this) {


These would actually be enforced by the type system already because they're string literal types

jelbourn · 2020-06-15T21:17:33Z

src/cdk-experimental/menu/menu-item.ts

+
+  /** Configure event subscriptions */
+  ngAfterContentInit() {
+    if (this.role !== 'menuitem') {


Rather than using conditionals to check for the type of the menu item throughout this PR, what do you think of having separate subclasses for menuitemradio and menuitemcheckbox? I have a hunch it will make the code simpler. They might even share a base class (or interface?) like MenuItemWithSelection.

Something like, CdkMenuItem, CdkMenuItemTrigger, CdkMenuItemRadio, CdkMenuItemCheckbox with CdkMenuItem being the base class and having a MenuItem interface?

Edit agreed offline to address this in a follow up PR

I was imagining one of these options:

The regular menu item isn't necessarily a trigger for another menu, it can just be a regular action, so the trigger would be a separate directive that's applied to a menu item. For the checkbox and radio items, they probably share some behavior, but I'm not sure how much. If it's a lot, I would add a base class for that shared code. If it's not a lot of code, then they would just extend CdkMenuItem directly.

jelbourn · 2020-06-15T21:19:18Z

src/cdk-experimental/menu/menu.ts

-  @Output() change: EventEmitter<CdkMenuItem>;
+  /** All the child MenuItem components which this directive wraps including descendants */
+  @ContentChildren(CdkMenuItem, {descendants: true})
+  readonly _allItems: QueryList<CdkMenuItem>;


Yeah, we don't use that convention yet because we haven't enabled --strictPropertyInitialization w/ TypeScript. We will likely do this someday, but don't have the assertion system set up presently.

jelbourn · 2020-06-15T21:21:47Z

src/cdk-experimental/menu/menu-item.ts

  /** Emits when the attached submenu is opened */
  @Output() opened: EventEmitter<void> = new EventEmitter();

+  constructor(
+    /** reference a parent CdkMenuGroup component */
+    private readonly _menuGroup: CdkMenuGroup


I know we discussed this at some point, but we can do it more here- where did we land on making it so that the menu/items only know about each other in one direction? E.g. the menu know about its items OR the items know about their menu, but not both.

We decided to use an interface which I've started implementing for the next feature in order to prevent circular dependencies
Edit: Decided to go one way - most likely the parent knows about children

… MenuBar

…endency issue

Multiple MenuItems may be loaded on a loop so we want to unsubscribe and not rely on parents destruction

…sed fixtures

…array types

jelbourn

LGTM, can explore different classes for the different types of items in a follow-up PR

angular-automatic-lock-bot · 2020-07-18T00:06:54Z

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

andy9775 requested a review from jelbourn as a code owner June 12, 2020 18:52

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

andy9775 requested a review from teflonwaffles June 12, 2020 18:53

teflonwaffles reviewed Jun 12, 2020

View reviewed changes

src/cdk-experimental/menu/menu-bar.spec.ts Outdated Show resolved Hide resolved

teflonwaffles reviewed Jun 12, 2020

View reviewed changes

andy9775 added 5 commits June 12, 2020 17:04

refactor(cdk-experimental/menu): Mark unchnaged property as readonly

c230e82

refactor(cdk-experimental/menu): Mark MenuItem panel reference optional

bb7b960

Not all MenuItems will open a submenu so this updates the reference to being optional.

fix(cdk-experimental/menu): Fix circular dependency

56f724e

Use MenuItem to help fix the circular dependency between CdkMenuItem and CdkMenuGroup. The interface is also the return type the change event handler and therefore only specifies relevant methods/properties

Merge branch 'master' of github.com:angular/components into cdk-menu-…

39a3d88

…group

refactor(cdk-experimental/menu): Remove unncessary selectors from tes…

0438e6c

…t fixtures

teflonwaffles reviewed Jun 12, 2020

View reviewed changes

andy9775 added 3 commits June 13, 2020 14:56

refactor(cdk-experimental/menu): Move logic for clicke event emitting

463f5ba

CdkMenuGroup should determine when to emit change events on its change emitter and not the MenuItem

test(cdk-experimental/menu): Reduce unnecessary sanity tests for Menu…

561040c

… and MenuGroup

test(cdk-experimental/menu): Remove usage of private members in menu …

f1949de

…tests

jelbourn reviewed Jun 15, 2020

View reviewed changes

andy9775 added 6 commits June 15, 2020 17:39

refactor(cdk-experimental/menu): Remove unused properties on Menu and…

25baedd

… MenuBar

build: CdkMenuItem now uses an interface which fixes the circular dep…

120139c

…endency issue

bug(cdk-experimental): Unsubscribe MenuItem subscriptions

95f0747

Multiple MenuItems may be loaded on a loop so we want to unsubscribe and not rely on parents destruction

style: Remove parens around single argument arrow function

a412360

test(cdk-experimental/menu): refactor tests to use smaller, more focu…

8c9edc8

…sed fixtures

style(cdk-experimental/menu): Use T[] syntax instead of Array<T> for …

9714c83

…array types

andy9775 requested a review from a team as a code owner June 16, 2020 15:10

jelbourn approved these changes Jun 16, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker labels Jun 16, 2020

jelbourn added merge safe target: patch This PR is targeted for the next patch release labels Jun 16, 2020

teflonwaffles approved these changes Jun 16, 2020

View reviewed changes

andy9775 added target: minor This PR is targeted for the next minor release and removed target: patch This PR is targeted for the next patch release labels Jun 17, 2020

mmalerba merged commit d8c2d63 into angular:master Jun 17, 2020

mmalerba mentioned this pull request Jun 19, 2020

refactor(cdk-experimental/menu): Break MenuItems into directives #19688

Merged

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jul 18, 2020

		@@ -0,0 +1,175 @@
		import {ComponentFixture, TestBed, async} from '@angular/core/testing';

feat(cdk-experimental/menu): Implement grouping logic for menuitems #19628

feat(cdk-experimental/menu): Implement grouping logic for menuitems #19628

Uh oh!

Conversation

andy9775 commented Jun 12, 2020

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andy9775 Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

andy9775 Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

andy9775 Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

andy9775 Jun 12, 2020 •

edited

Loading

andy9775 Jun 12, 2020 •

edited

Loading

andy9775 Jun 12, 2020 •

edited

Loading

andy9775 Jun 15, 2020 •

edited

Loading

andy9775 Jun 15, 2020 •

edited

Loading