feat(cdk-experimental/menu): implement menu handling logic #19701

andy9775 · 2020-06-20T00:53:09Z

Implement logic to open/close attached menus and submenus when triggering the specified MenuItem to which the menu is attached to

CdkMenu elements should be wrapped with ng-template marked with a CdkMenuPanel directive

Implement logic to open/close attached menus and submenus when triggering the specified MenuItem to which the menu is attached to

teflonwaffles · 2020-06-22T17:06:22Z

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

+    let triggers: CdkMenuItemTrigger[];
+    let nativeTriggers: HTMLButtonElement[];
+
+    const setElements = () => {


This method name seems ambiguous. Could you use something more specific, like 'grabElementsForTesting'?

teflonwaffles · 2020-06-22T17:10:18Z

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

+      detectChanges();
+
+      expect(menus.length).toEqual(1);
+      expect(triggers[0]._menuPanel!._menu!).toEqual(menus[0]);


I'm unclear how this tests the condition stated in the test title. Could you confirm that this assertion is necessary?

I check that there is only one opened menu and that the opened menu is the one which is attached to the trigger.

But since the 'menus' array is populated dynamically, is that enough to verify that the correct menu is opened? (If _menuPanel on the trigger is known to refer to the correct menu, before the menu is opened, then that would do it.)

ya, so the _menuPanel menu reference doesn't change since it's set in the template.

Looking at it now they should be flipped though - that's probably where the confusion is.

teflonwaffles · 2020-06-22T17:25:33Z

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

+      triggers[1].toggle();
+      detectChanges();
+
+      expect(menus.length).withContext('first level menu should stay open').toEqual(1);


If you really want to get picky, it might be nice to test that the nested submenu is the one that gets closed (as opposed to the outer menu getting closed, and the nested submenu dangling from the outer trigger). I'm not sure how you'd do that, though. (Also, since unit tests can assume some knowledge of implementation details, it's probably okay to skip this if it's infeasible based on the implementation.)

I can verify that the remaining menu is the one which is attached to the first trigger. This will ensure that the first submenu is left open.

teflonwaffles · 2020-06-22T17:34:34Z

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

+
+    it('should fallback to positioning the overlay above the trigger for horizontal Menu', () => {
+      nativeTriggers[0].style.position = 'fixed';
+      nativeTriggers[0].style.bottom = '0';


Does this put the trigger at the bottom of the page? (I've been under the impression 0 is the top of the page, but I'll admit my CSS isn't very good.)

I'd make the test description clearer by making the condition explicit. ('should put overlay above trigger for horizontal Menu if no room below', or something like that.)

Since the position is set to fixed, setting the bottom to 0 sets it 0px away from the bottom edge. Similar logic applies for right below.

Reference https://developer.mozilla.org/en-US/docs/Web/CSS/bottom and https://developer.mozilla.org/en-US/docs/Web/CSS/right

I'll clear up the comment though

Wow, I had no idea that was a thing. CSS is a mystery...

teflonwaffles · 2020-06-22T17:37:13Z

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

+
+    it('should fallback to positioning nested submenu overlay to the left in ltr layout', () => {
+      nativeTriggers[0].style.position = 'fixed';
+      nativeTriggers[0].style.right = '0';


Again, I thought 0 was the left.

teflonwaffles · 2020-06-22T17:54:55Z

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

+
+  /** Return the portal to be attached to the overlay which contains the menu */
+  private _getPortal() {
+    if (!this._portal || this._portal.templateRef !== this._menuPanel?._templateReference) {


I assume the right hand side of this OR statement would happen if the trigger's submenu programmatically changes (via an *ngIf, for instance)? If so, it may be a good idea to make this explicit in the method comment.

teflonwaffles · 2020-06-22T18:01:46Z

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

+
+  constructor(readonly _templateReference: TemplateRef<unknown>) {}
+
+  /** Set the menu component on the menu panel */


Could you make this method description a little more elaborate? Right now, it doesn't actually convey why this method is important.

jelbourn · 2020-06-22T19:33:57Z

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

+export const CDK_MENU = new InjectionToken<Menu>('cdk-menu');
+
+/** Interface which specifies Menu operations and used to break circular dependency issues */
+export interface Menu {


We should make sure that this symbol isn't exported through the public API when we get that far

jelbourn · 2020-06-22T19:40:00Z

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

+  @Output() readonly opened: EventEmitter<void> = new EventEmitter();
+
+  /** Emits when the attached submenu is requested to close  */
+  @Output() readonly closed: EventEmitter<void> = new EventEmitter();


I think these outputs should have a prefixes, like cdkMenuOpened / cdkMenuClosed.

If we were really technical, the names would be e.g. cdkMenuTriggerOpened, but I don't think this actually makes sense.

jelbourn · 2020-06-22T19:45:10Z

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

+  @Output() readonly closed: EventEmitter<void> = new EventEmitter();
+
+  /** A reference to the overlay which manages the triggered submenu */
+  private _overlayReference: OverlayRef | null = null;


Just _overlayRef is okay since the concept of a "ref" is widespread throughout Angular

jelbourn · 2020-06-22T19:46:05Z

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

@@ -24,29 +42,121 @@ import {CdkMenuItem} from './menu-item';
  exportAs: 'cdkMenuTriggerFor',
  host: {
    'aria-haspopup': 'menu',
+    '[attr.aria-expanded]': 'isSubmenuOpen() || null',


So, opposite of other attributes, I think we do want aria-expanded to always be present because it cues the user that there's something here to be expanded (otherwise it seems like a regular button)

jelbourn · 2020-06-22T19:51:25Z

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

+    }
+  }
+
+  /** Return the configuration object used to create the overlay */


nit: Prefer "Gets ..." to "Returns ..."
(here and elsehwere)

jelbourn · 2020-06-22T19:54:10Z

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

+          {originX: 'end', originY: 'top', overlayX: 'start', overlayY: 'top'},
+          {originX: 'end', originY: 'bottom', overlayX: 'start', overlayY: 'bottom'},
+          {originX: 'start', originY: 'top', overlayX: 'end', overlayY: 'top'},
+          {originX: 'start', originY: 'bottom', overlayX: 'end', overlayY: 'bottom'},


It would be good to add a TODO here to one common "source of truth" for the conventional menu/dropdown positioning strategy, potentially in cdk/overlay

In ViewEngine, if a directive is placed on an `ng-template` a child cannot reference it via DI. Therefore when running under VE a reference to the parent CdkMenuPanel must be explicitly passed to the child CdkMenu. Under Ivy it defaults to the injected value.

root trigger

*Ref is a common naming scheme throughout angular therefore there is no need to specify *Reference

Should be able to use a common configuration strategy as defined in cdk/overlay (ideally) since most components will prefer a common set of layouts

true/false

andy9775 · 2020-06-23T00:40:34Z

@jelbourn @teflonwaffles comments should be addressed. PTAL

It should assert that the dynamically opened menu is expected to be the one attached to the trigger

teflonwaffles · 2020-06-23T15:43:45Z

src/cdk-experimental/menu/menu.ts

@@ -48,10 +56,42 @@ export class CdkMenu extends CdkMenuGroup implements AfterContentInit {
  @ContentChildren(CdkMenuGroup, {descendants: true})
  private readonly _nestedGroups: QueryList<CdkMenuGroup>;

+  /**
+   * A reference to the enclosing parent menu panel.


Just to clarify, what's the difference between cdkMenu and cdkMenuPanel?

cdkMenu is a directive attached directly to the menu content. cdkMenuPanel is attached to the ng-template.

cdkMenuPanel provides a reference to the template used to display the content (cdkMenu) in an overlay. It also provides a reference to the child menu.

teflonwaffles · 2020-06-23T15:47:53Z

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

+        .toEqual(Math.floor(nativeMenus[1].getBoundingClientRect().left));
+    });
+
+    it('should position nested submenu at trigger level by default', () => {


Is there a fallback test for this?

No. My thinking is if the left/right fallback works testing left/right height would start testing the implementation of the Overlay class.

jelbourn · 2020-06-23T16:14:20Z

src/cdk-experimental/menu/menu.ts

+    if (parent) {
+      parent._registerMenu(this);
+    } else {
+      throwMissingMenuPanelError();


This wouldn't strictly be an error- using a menu outside of a panel should be valid (i.e. having an inline menu)

Should I remove this now or address it once I implement inline menu support?

SGTM to do in a follow-up PR

jelbourn · 2020-06-23T16:18:10Z

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

+  /** The Portal in which the Menu is displayed inside of */
+  private _portal: TemplatePortal;


Suggested change

/** The Portal in which the Menu is displayed inside of */

private _portal: TemplatePortal;

/** The content of the menu panel opened by this trigger. */

private _panelContent: TemplatePortal;

jelbourn · 2020-06-23T16:21:25Z

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

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


Thinking about this more, should this just be hasMenu and isMenuOpen below? Each trigger is inherently connected to a single menu item, regardless of whether it's a top-level menu or a sub-menu on a menu item (I think the class name also then becomes just CdkMenuTrigger)

Since cdkMenuTrigger is added alongside cdkMenuItem would CdkMenuItem also drop the Sub?

I think it makes sense to stick with one approach for consistency since it becomes confusing why a MenuItem has a submenu and a trigger has a Menu.

SGTM to drop "sub" everywhere in a follow-up PR

jelbourn · 2020-06-23T16:21:52Z

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

+
+  /** Determine and return where to position the submenu relative to the menu item */
+  private _getOverlayPositions(): ConnectedPosition[] {
+    // TODO: use a common positioning strategy from (possibly) cdk/overlay


Suggested change

// TODO: use a common positioning strategy from (possibly) cdk/overlay

// TODO: use a common positioning config from (possibly) cdk/overlay

andy9775 · 2020-06-23T19:40:10Z

@jelbourn @teflonwaffles Followup feedback should be addressed PTAL

jelbourn

Looks good, just one last comment

jelbourn · 2020-06-23T22:09:52Z

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

+  @Output() readonly cdkMenuOpened: EventEmitter<void> = new EventEmitter();
+
+  /** Emits when the attached submenu is requested to close  */
+  @Output() readonly cdkMenuClosed: EventEmitter<void> = new EventEmitter();


Ah, I should have been more specific here; these can be

@Output('cdkMenuOpened') readonly opened: EventEmitter<void> = new EventEmitter(); @Output('cdkMenuClosed') readonly closed: EventEmitter<void> = new EventEmitter();

We always want the actual property in TypeScript to be unprefixed, with the prefix only aplpying to template bindings via the alias

🤣 makes sense now.

Addressed

jelbourn · 2020-06-23T22:12:26Z

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

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


SGTM to drop "sub" everywhere in a follow-up PR

jelbourn · 2020-06-23T22:12:51Z

src/cdk-experimental/menu/menu.ts

+    if (parent) {
+      parent._registerMenu(this);
+    } else {
+      throwMissingMenuPanelError();


SGTM to do in a follow-up PR

with prefix - not internal name

jelbourn

LGTM

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

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 added 2 commits June 19, 2020 20:43

test(cdk-experimental/menu): wrap CdkMenu with CdkMenuPanel

daa5732

CdkMenu elements should be wrapped with ng-template marked with a CdkMenuPanel directive

feat(cdk-experimental/menu): implement menu handling logic

9c12cde

Implement logic to open/close attached menus and submenus when triggering the specified MenuItem to which the menu is attached to

andy9775 requested a review from jelbourn as a code owner June 20, 2020 00:53

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

andy9775 requested a review from teflonwaffles June 20, 2020 00:53

andy9775 added 2 commits June 19, 2020 21:12

docs(cdk-experimental/menu): add missing license header

cf007c7

test(cdk-experimental/menu): output template on ng-container not div

73e9eb1

teflonwaffles reviewed Jun 22, 2020

View reviewed changes

jelbourn reviewed Jun 22, 2020

View reviewed changes

andy9775 added 12 commits June 22, 2020 16:56

test(cdk-experimental/menu): fix ViewEngine test failures

22ba67c

refactor(cdk-experimental/menu): rename test config function

3cf95d7

test(cdk-experimental/menu): test that remaining menu is attached to

12cf65a

root trigger

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

78ccf28

docs(cdk-experimental/menu): clear up method doc

f95085a

refactor(cdk-experimental/menu): rename emitters to be more explicit

c6a20fd

refactor(cdk-experimental/menu): rename properties to *Ref

f1356e2

*Ref is a common naming scheme throughout angular therefore there is no need to specify *Reference

docs(cdk-experimental/menu): prefer to use get over return in docs

7345f57

docs(cdk-experimental/menu): add todo: overlay config

bcdcca4

Should be able to use a common configuration strategy as defined in cdk/overlay (ideally) since most components will prefer a common set of layouts

docs(cdk-experimental/menu): clarify the use of overlay portal

be99845

fix(cdk-experimental/menu): aria-expanded should be explicitly

96eedc7

true/false

andy9775 requested review from jelbourn and teflonwaffles June 23, 2020 00:45

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

4962a59

It should assert that the dynamically opened menu is expected to be the one attached to the trigger

teflonwaffles reviewed Jun 23, 2020

View reviewed changes

jelbourn reviewed Jun 23, 2020

View reviewed changes

andy9775 added 2 commits June 23, 2020 15:30

refactor(cdk-experimental/menu): rename portal reference

48e2b79

refactor(cdk-experimental/menu): clarify todo comment

96208b1

jelbourn reviewed Jun 23, 2020

View reviewed changes

refactor(cdk-experimental/menu): explicitly set the output emitter name

0bfb61e

with prefix - not internal name

andy9775 requested review from jelbourn and teflonwaffles June 23, 2020 22:50

teflonwaffles approved these changes Jun 23, 2020

View reviewed changes

jelbourn approved these changes Jun 24, 2020

View reviewed changes

jelbourn added lgtm action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jun 24, 2020

jelbourn merged commit b3d6d92 into angular:master Jun 25, 2020

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


		constructor(readonly _templateReference: TemplateRef<unknown>) {}

		/** Set the menu component on the menu panel */

		/** The Portal in which the Menu is displayed inside of */
		private _portal: TemplatePortal;

	// TODO: use a common positioning strategy from (possibly) cdk/overlay
	// TODO: use a common positioning config from (possibly) cdk/overlay

feat(cdk-experimental/menu): implement menu handling logic #19701

feat(cdk-experimental/menu): implement menu handling logic #19701

Uh oh!

Conversation

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

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

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 23, 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!

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

andy9775 commented Jun 23, 2020

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

andy9775 Jun 23, 2020 •

edited

Loading

andy9775 Jun 23, 2020 •

edited

Loading