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

andy9775 · 2020-06-18T14:11:09Z

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.

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.

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.

Element attributes in the host bindings are inherited by subclasses with Ivy but not View Engine. Specifying explicitly on each subclass fixes this issue.

teflonwaffles · 2020-06-18T15:45:40Z

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

-        .queryAll(By.directive(CdkMenuItem))
-        .map(e => e.injector.get(CdkMenuItem));
+        .queryAll(By.directive(CdkMenuItemRadio))
+        .map(e => e.injector.get(CdkMenuItemRadio));


Here and elsewhere, the test code may be a little clearer if you replace 'e' with a more descriptive name (such as debugElement or menuItemRadioDebugElement). If you're following the CDK convention, however, you should probably continue doing so.

This convention is totally acceptable (even encouraged) in this repo. I find that, for simple map/find/filter functions, one-character names make the operation easier to read.

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

teflonwaffles · 2020-06-18T15:50:00Z

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

+  private readonly _selectableItems: QueryList<CdkMenuItemSelectable>;
+
+  /** Emitter used to unsubscribe from selectable item click events */
+  private readonly _nextChanges: EventEmitter<void> = new EventEmitter();


I'd give this a clearer name. Maybe '_unsubscribeCurrentMenuItemListeners'?

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

teflonwaffles · 2020-06-18T16:06:53Z

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

+    expect(spy).toHaveBeenCalledTimes(0);
+  });
+
+  it('should toggle state when called with own id and differing id', () => {


'and not with differing id'?

teflonwaffles · 2020-06-18T16:12:08Z

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

+    }
+  }
+
+  static ngAcceptInputType_checked: BooleanInput;


I've never seen a static class member used in TypeScript before. What is this for?

when you do something like <div disabled> this is the equivalent of disabled = "" and therefore we need to coerce the truthy value. However we want checked to specify that it is a boolean type. This ensures that we can provide a truthy type but allows us to specify it as a boolean type.

Yep- it let's Angular know that, when binding a value from a template, the property accepts a broader range of values than its setter specifies.

teflonwaffles · 2020-06-18T16:19:17Z

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

+  }
+
+  /** Return true if the trigger has an attached menu */
+  hasSubmenu() {


Should this be private? (It's odd for 'hasSubmenu' to be a field in some places and a method in others.)

teflonwaffles · 2020-06-18T16:24:45Z

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

+
+      menuItems.forEach(menuItem => menuItem.trigger());
+
+      expect(spy).toHaveBeenCalledTimes(0);


Optional: .not.toHaveBeenCalled() may be mildly more readable.

teflonwaffles · 2020-06-18T16:30:14Z

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

+      menuItems = getMenuItems();
+    }));
+
+    it('should not emit after the group mounts', () => {


This terminology is unclear. Instead of 'mounts', could you use terms more specific to the directives? (Also, I'm not sure which statement corresponds to the 'mounting' in the test code.)

teflonwaffles · 2020-06-18T16:35:56Z

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

+      <ng-template [ngIf]="!show">
+        <li><button cdkMenuItemCheckbox>first</button></li>
+      </ng-template>
+      <ng-template [ngIf]="show">


Ah, now I see -- 'show = true' is what mounts the inner group. However, does the test depend on 'first' being hidden when show is true? If not, I'd remove the condition from 'first'.

I'd use a clearer name for 'show' so the test code is clearer -- maybe 'showInnerGroup' or 'renderInnerGroup'?

Also, is there a reason you're using the more verbose [ngIf] syntax? (If there is, I'd add a comment explaining the reason, unless it's part of CDK convention.)

teflonwaffles · 2020-06-18T16:40:06Z

src/cdk-experimental/menu/menu.ts

+    // If there are nested MenuGroup elements within this group
+    // we complete the change emitter in order to ensure that
+    // the this menu does not emit change events.
+    if (this._hasNestedGroups()) {


Is the if-then necessary here? I would expect the behavior to be the same even without it, assuming _nestedGroups.changes emits immediately if there are nested groups from the beginning. (If that assumption isn't correct, then it does become necessary. I might add a comment explaining this, if that's the case.)

Here I'm saying, if we have groups from the start lets close the emitter. Otherwise we don't have any on initial mount, but if some do show up, lets disable the emitter once they do.

Right, so could this work without the if statement, by simply relying on the subscription?

There's unfortunately no way to subscribe to the QueryList in such a way where your callback will get executed when the initial children are mounted. At the time you call subscribe the QueryList will have elements if they are already visible in the Component. Hence any logic needs to perform some logic on the existing children (if any) and subscribe to any future changes.

For example, if we have a single CdkMenuGroup with no conditional mounting the QueryList will contain that one component when ngAfterContentInit executes but the changes emitter won't emit until another is added, or the current one is removed. That means that if there is only ever one CdkMenuGroup, the changes emitter won't execute at all.

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.

Change event emitter emits when the list of Selectables gets updated

andy9775 · 2020-06-18T21:36:24Z

@teflonwaffles the feedback should be addressed. As for the forEach, I've seen it throughout the repo but I'll let Jeremy respond as well.

compileComponents() is an async function and therefore createComponents cannot follow it. Breaking up into seperate beforeEach blocks prevents any potential issues

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

jelbourn · 2020-06-18T23:29:06Z

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

+    this._selectableItems.changes.subscribe((selectableItems: QueryList<CdkMenuItemSelectable>) => {
+      this._selectableChanges.next();
+      selectableItems.forEach(selectable => this._registerClickListener(selectable));
+    });


Could you move the logic inside ngAfterContentInit into a separate method? Something like

ngAfterContentInit() { this._registerMenuSelectionListeners(); }

This related to my philosophy on naming methods for what they do rather than how/when they're called. This also helps things that you would call from lifecycle hooks more single-responsibility

jelbourn · 2020-06-18T23:33:58Z

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

+      .query(By.directive(CdkMenuItemCheckbox))
+      .injector.get(CdkMenuItemCheckbox);
+
+    nativeButton = fixture.debugElement.query(By.directive(CdkMenuItemCheckbox)).nativeElement;


I typically would name these something like checkbox and checkboxElement

jelbourn · 2020-06-18T23:35:03Z

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

+    '(click)': 'trigger()',
+    'type': 'button',
+    'role': 'menuitemcheckbox',
+    '[attr.aria-checked]': 'checked',


Based on screen-reader testing, we may want aria-checked to also be checked || null

Since it's a checkbox wouldn't it be explicitly checked or not?
Edit: Wouldn't we want to specify false instead of null for unchecked state?

We've heard some feedback that making elements null when they would be false makes the screen-reader less verbose, so rather then being e.g.

Cheese selected Tomato not selected Onion not selected Pepper selected

it would instead read

Cheese selected Tomato Onion Pepper selected

So that "not selected" is implicit

jelbourn · 2020-06-18T23:48:14Z

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

+    }
+  }
+
+  static ngAcceptInputType_checked: BooleanInput;


Yep- it let's Angular know that, when binding a value from a template, the property accepts a broader range of values than its setter specifies.

jelbourn · 2020-06-18T23:57:19Z

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

+/** Counter used to set a unique name for a selectable item */
+let _uniqueNameCounter = 0;
+/** Counter used to set a unique id for a selectable item */
+let _uniqueIdCounter = 0;


You could use the same counter for each of these. Also they don't need to start with _ since they're not exported. I typically just call it nextId

jelbourn · 2020-06-19T00:06:47Z

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

+      const spy = jasmine.createSpy('changeSpy for root menu');
+      menu.change.subscribe(spy);
+
+      menuItems.forEach(menuItem => menuItem.trigger());


Can use for of here instead of .forEach

jelbourn · 2020-06-19T00:07:17Z

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

+      fixture = TestBed.createComponent(MenuWithConditionalGroup);
+
+      fixture.detectChanges();
+
+      menu = fixture.debugElement.query(By.directive(CdkMenu)).injector.get(CdkMenu);
+
+      menuItems = getMenuItems();


Suggested change

fixture = TestBed.createComponent(MenuWithConditionalGroup);

fixture.detectChanges();

menu = fixture.debugElement.query(By.directive(CdkMenu)).injector.get(CdkMenu);

menuItems = getMenuItems();

fixture = TestBed.createComponent(MenuWithConditionalGroup);

fixture.detectChanges();

menu = fixture.debugElement.query(By.directive(CdkMenu)).injector.get(CdkMenu);

menuItems = getMenuItems();

Maybe a bit overboard with the empty lines here

jelbourn · 2020-06-19T00:08:54Z

src/cdk-experimental/menu/menu.ts

+      // view engine has a bug where @ContentChildren will return the current element
+      // along with children if the selectors match - not just the children.
+      // Here we check to see if the first element is a CdkMenu in order to ensure
+      // that we return true iff there are child CdkMenuGroup elements.


I would avoid putting comments in the conditional expression, instead putting the comment above the return

Extract logic from the lifecycle method and place into a more readable method

radio

jelbourn

LGTM

jelbourn · 2020-06-19T17:15:37Z

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

+    '(click)': 'trigger()',
+    'type': 'button',
+    'role': 'menuitemcheckbox',
+    '[attr.aria-checked]': 'checked',


We've heard some feedback that making elements null when they would be false makes the screen-reader less verbose, so rather then being e.g.

Cheese selected Tomato not selected Onion not selected Pepper selected

it would instead read

Cheese selected Tomato Onion Pepper selected

So that "not selected" is implicit

not false

mmalerba · 2020-06-19T20:53:33Z

this builds on #19628 which was target: minor, switching this one to target: minor as well

angular-automatic-lock-bot · 2020-07-21T00:07:26Z

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 18, 2020 14:11

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

andy9775 requested a review from teflonwaffles June 18, 2020 14:11

andy9775 added 2 commits June 18, 2020 11:06

test(cdk-experimental/menu): Remove unnecessary tests

20a9915

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

e929ad8

Element attributes in the host bindings are inherited by subclasses with Ivy but not View Engine. Specifying explicitly on each subclass fixes this issue.

teflonwaffles reviewed Jun 18, 2020

View reviewed changes

src/cdk-experimental/menu/menu-group.ts Show resolved Hide resolved

teflonwaffles reviewed Jun 18, 2020

View reviewed changes

src/cdk-experimental/menu/menu-group.ts Show resolved Hide resolved

teflonwaffles reviewed Jun 18, 2020

View reviewed changes

andy9775 added 5 commits June 18, 2020 14:56

test(cdk-experimental): Clear up tests

96a5e9e

refactor(cdk-experimental/menu): Make internal method private

0d7f807

test(cdk-experimental/menu): clear up test description

988412d

refactor(cdk-experimental/menu): Rename change event emitter

a5303e8

Change event emitter emits when the list of Selectables gets updated

andy9775 requested a review from teflonwaffles June 18, 2020 21:36

test(cdk-experimental/menu): Refactor test configuration

cc2cfd8

compileComponents() is an async function and therefore createComponents cannot follow it. Breaking up into seperate beforeEach blocks prevents any potential issues

jelbourn reviewed Jun 19, 2020

View reviewed changes

andy9775 added 4 commits June 18, 2020 20:19

refactor(cdk-experimental/menu): Rename registration method

400e55c

Extract logic from the lifecycle method and place into a more readable method

refactor(cdk-experimental/menu): rename native elements for clarity

80b9bcd

refactor(cdk-experimental/menu): use the same counter for name and id

f9825f5

style(cdk-experimental/menu): clean up whitespace

33513a3

andy9775 added 2 commits June 18, 2020 20:38

test(cdk-experimental/menu): refactor to use for..of where prefered

96bf59d

style(cdk-experimental): move comment outside of conditional block

042e321

andy9775 requested a review from jelbourn June 19, 2020 00:42

andy9775 added 3 commits June 18, 2020 20:54

refactor(cdk-experimental/menu): move logic out of lifecycle methods

299db25

refactor(cdk-experimental/menu): simplify checkbox toggle logic

97db1ae

docs(cdk-experimental/menu): add clearer documentation for checkbox and

26f90b6

radio

jelbourn approved these changes Jun 19, 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 labels Jun 19, 2020

refactor(cdk-experimental/menu): refactor checked attribute to be null

919dc03

not false

mmalerba 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 19, 2020

mmalerba merged commit 298cdc0 into angular:master Jun 19, 2020

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


		menuItems.forEach(menuItem => menuItem.trigger());

		expect(spy).toHaveBeenCalledTimes(0);

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

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

Uh oh!

Conversation

andy9775 commented Jun 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

teflonwaffles Jun 18, 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!

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

Uh oh!

andy9775 commented Jun 18, 2020

Uh oh!

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!

andy9775 Jun 19, 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!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mmalerba commented Jun 19, 2020

Uh oh!

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

Uh oh!

Uh oh!

teflonwaffles Jun 18, 2020 •

edited

Loading

andy9775 Jun 19, 2020 •

edited

Loading