feat(menu): add nested menu functionality #5493

crisbeto · 2017-07-04T19:48:30Z

Adds the ability for an md-menu-item inside of a md-menu to trigger another md-menu. This is a first step towards a md-toolbar component.

Demo: https://material-cb7ec.firebaseapp.com/menu

Fixes #1429.

m98 · 2017-07-05T09:24:02Z

Would not be good if we could show a simple arrow (points to the right) for nested menus? The user should not guess that does this menu item has any nested menu or not, and do this continuously for the whole list items. (Material source has size definitions for menu items' icons on right side; It can be the icon that points to right and shows that the item has nested menu)
The animation seems to run very fast after mouse-in (no delay), a short delay would make it feel better for users, the reason I say it's not good to open nested menu too fast and with no delay is that the user is just mouse hovering the menu items to find the item he/she is looking for, and the fast opening menu (without delay) is disturbing. (live: animate speed and delay from Google Docs demo)
(See this short video)

joejordanbrown · 2017-07-05T12:14:06Z

We need to make sure that when using a mouse and running the cursor diagnally that it doesn't change to another nested menu item.

I haven't seen this mentioned in the design document, so I'm not sure if this has been overlooked at the moment.

The google documents menu does this but i beleive its done using a timer instead of detecting direction, a good example would be the amazon menu even if you move slowly it still has the correct nested menu open and moving the cursor up and down the menu shows the nested items without delay. This seems to be a lot more user friendly.

This page here documents it.

jelbourn · 2017-07-06T21:37:28Z

The article @joejordanbrown posted makes a pretty good point; we should consider adding a feature like that to the cdk and using in the menu as a follow-up.

crisbeto · 2017-07-06T21:43:46Z

Makes sense, I'll add it to my backlog since this PR is big enough as it is.

jelbourn

Did you plan on doing anything for adding an indicator for nested menu triggers in a follow-up?

jelbourn · 2017-07-06T21:40:12Z

src/lib/menu/menu-directive.ts

+  /** Stream that emits whenever the hovered menu item changes. */
+  get hover(): Observable<MdMenuItem> {
+    return merge(...this.items.map(item => item.hover));
+  }


Make this a function to avoid the compiled getter boilerplate?

jelbourn · 2017-07-06T21:42:00Z

src/lib/menu/menu-item.ts

-export class MdMenuItem extends _MdMenuItemMixinBase implements Focusable, CanDisable {
+export class MdMenuItem extends _MdMenuItemMixinBase implements Focusable, CanDisable, OnDestroy {
+  /** Stream that emits when the menu item is hovered. */
+  hover: Subject<MdMenuItem> = new Subject();


Is this meant to be an internal api?

It is mainly to facilitate the nested menu, but I can see it being useful in other cases as well.

jelbourn · 2017-07-06T21:50:12Z

src/lib/menu/menu-trigger.ts

+  /** Whether the menu triggers a sub-menu or a top-level one. */
+  get triggersSubmenu(): boolean {
+    return !!(this._menuItemInstance && this._parentMenu);
+  }


Make this a function?

jelbourn · 2017-07-06T22:04:50Z

src/lib/menu/menu-trigger.ts


-    if (!this.menu.overlapTrigger) {
+    if (this.triggersSubmenu) {


Can you add some comments around how the positioning is different when triggering a submenu?

crisbeto · 2017-07-07T14:56:47Z

I'll add some UX improvements in a follow-up. This PR is mainly for figuring out the positioning, accessibility and opening/closing logic.

crisbeto · 2017-07-07T18:22:14Z

Addressed the feedback @jelbourn.

jelbourn

Looks good to me, but I want @kara to take a look when she gets back next week

Splaktar · 2017-07-07T21:15:48Z

I agree with @m98 that the animation speed should be slowed down from what the demo has. It should match what the first level menu uses.

xuanshenbo · 2017-07-09T22:52:19Z

wrt adding an indicator, could be related to #5067

kara

Looks good, a couple comments.

Also, a few things to track for future improvement PRs:

Agree that the sub-menu triggers need arrow icons for usability. It's not clear which menu items are submenus otherwise. Create an issue for this?
Moving the mouse quickly to the left/right after opening the top level menu can mess up the positioning. Look into this in a future PR?

kara · 2017-07-17T22:49:40Z

src/lib/menu/menu.md

+
+### Nested menu
+
+Material supports the ability for a `md-menu-item` to open a sub-menu. To do so, you have to define


Typo: a md-menu-item -> an md-menu-item

kara · 2017-07-18T00:03:15Z

src/lib/menu/menu-trigger.ts

+    this._parentMenuSubscriptions.push(this._parentMenu.close.subscribe(() => {
+      this.menu.close.emit();
+    }));
+  }


Is it at all possible to avoid having a list of subscriptions? I think the logic might be neater / easier to follow if we merge the streams that result in the menu closing into one subscription, rather than having a few separate subscriptions that do the same thing.

get _menuClosingActions() { return merge( this._overlayRef.backdropClick, this._parentMenu.close, this._parentMenu.hover().filter(active => active !== this._menuItemInstance); }

Then instead of only subscribing to the backdrop when the menu opens, we subscribe to the menu closing events when the menu opens, kind of like the autocomplete.

_subscribeToMenuClose() { this._closingSubscription = this._menuClosingActions.subscribe(() => this.menu.close.emit()); }

Ultimately, we'd still be adding another subscription onInit for the parent menu hovering (to open the menu when the active item is this menu item), but at least we wouldn't be adding an array of subscriptions that we'd need to manage.

I did it initially, because they subscribe at different times in the lifecycle. Looking at it again, they don't really need to do that. Refactored.

Adds the ability for an `md-menu-item` inside of a `md-menu` to trigger another `md-menu`. This is a first step towards a `md-toolbar` component. Fixes angular#1429.

crisbeto · 2017-07-18T08:22:19Z

Rebased and addressed the feedback. Also I agree on the UX improvements, but I'll add them in a follow-up. The main purpose of this PR was to sort out the opening/closing logic, accessibility and positioning. Can you take another look @kara?

kara

Almost there; just noticed one more thing. When you click on a menu item, the parent menus don't seem to close (only its own parent menu). Seems like this should be fixed in this PR.

kara · 2017-07-18T16:19:30Z

src/lib/menu/menu-trigger.ts

+  /** Returns a stream that emits whenever an action that should close the menu occurs. */
+  private _menuClosingActions() {
+    const backdrop = this._overlayRef!.backdropClick();
+    const parentClose = !this._parentMenu ? observableOf(null) : this._parentMenu.close;


Nit: I'd switch the logic here and on line 364 because it's harder to scan "if not, else" statements (the else becomes "if not not").

this._parentMenu ? this._parentMenu.close : observableOf(null);

crisbeto · 2017-07-18T17:26:29Z

Good point WRT to closing all of the menu on click @kara. I've addressed it together with the other comment. Also redeployed the demo to https://material-cb7ec.firebaseapp.com/menu

kara

LGTM

kara · 2017-07-20T17:44:01Z

Note: Addition of isSubmenu to the interface is a breaking change and will need to be updated internally before merge.

crisbeto · 2017-07-20T17:52:05Z

I suppose the same can be said about the direction and the signature change of close. Should I scope these changes to the MdMenu since they're there to facilitate the nested menus?

kara · 2017-07-20T17:53:30Z

@crisbeto No changes needed on your end; just a note to myself so I don't accidentally merge prematurely.

kara · 2017-07-20T18:15:03Z

@crisbeto Actually, chatted with @jelbourn and making isSubmenu optional would be even better.

crisbeto · 2017-07-20T18:23:32Z

Done.

angular-automatic-lock-bot · 2019-09-06T17:03:17Z

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

crisbeto added the pr: needs review label Jul 4, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jul 4, 2017

crisbeto force-pushed the nested-menu-2 branch from 6cf7d5b to db4751f Compare July 4, 2017 20:00

crisbeto assigned jelbourn and kara Jul 4, 2017

jelbourn reviewed Jul 6, 2017

View reviewed changes

jelbourn requested a review from kara July 6, 2017 22:08

crisbeto force-pushed the nested-menu-2 branch from db4751f to 0674a60 Compare July 7, 2017 18:22

jelbourn reviewed Jul 7, 2017

View reviewed changes

crisbeto force-pushed the nested-menu-2 branch from 0674a60 to 61239fd Compare July 15, 2017 06:40

kara reviewed Jul 18, 2017

View reviewed changes

kara assigned crisbeto and unassigned kara Jul 18, 2017

crisbeto added 2 commits July 18, 2017 10:21

feat(menu): add nested menu functionality

942768b

Adds the ability for an `md-menu-item` inside of a `md-menu` to trigger another `md-menu`. This is a first step towards a `md-toolbar` component. Fixes angular#1429.

refactor: address feedback

64c0830

crisbeto force-pushed the nested-menu-2 branch from 61239fd to 64c0830 Compare July 18, 2017 08:09

chore: test failure on smaller screens

ae2042f

kara reviewed Jul 18, 2017

View reviewed changes

fix: close entire menu chain if menu item is clicked

0701085

kara approved these changes Jul 18, 2017

View reviewed changes

kara unassigned jelbourn and crisbeto Jul 18, 2017

kara added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed pr: needs review labels Jul 18, 2017

kara assigned crisbeto Jul 20, 2017

kara removed the action: merge The PR is ready for merge by the caretaker label Jul 20, 2017

chore: make isSubmenu optional

f98ee42

crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 20, 2017

chore: also direction

a8b7647

kara removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Jul 20, 2017

kara merged commit 1e0c1fc into angular:master Jul 20, 2017

jelbourn mentioned this pull request Jul 21, 2017

md-menu #119

Closed

jelbourn mentioned this pull request Jan 16, 2018

Selecting item from nested mat-menu is not user-friendly #9428

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(menu): add nested menu functionality #5493

feat(menu): add nested menu functionality #5493

crisbeto commented Jul 4, 2017

m98 commented Jul 5, 2017 •

edited

Loading

joejordanbrown commented Jul 5, 2017

jelbourn commented Jul 6, 2017

crisbeto commented Jul 6, 2017 •

edited

Loading

jelbourn left a comment

jelbourn Jul 6, 2017

jelbourn Jul 6, 2017

crisbeto Jul 7, 2017

jelbourn Jul 6, 2017

jelbourn Jul 6, 2017

crisbeto commented Jul 7, 2017

crisbeto commented Jul 7, 2017

jelbourn left a comment •

edited

Loading

Splaktar commented Jul 7, 2017

xuanshenbo commented Jul 9, 2017

kara left a comment •

edited

Loading

kara Jul 17, 2017

kara Jul 18, 2017

crisbeto Jul 18, 2017

crisbeto commented Jul 18, 2017

kara left a comment

kara Jul 18, 2017

crisbeto commented Jul 18, 2017 •

edited

Loading

kara left a comment

kara commented Jul 20, 2017

crisbeto commented Jul 20, 2017

kara commented Jul 20, 2017

kara commented Jul 20, 2017

crisbeto commented Jul 20, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019


		### Nested menu

		Material supports the ability for a `md-menu-item` to open a sub-menu. To do so, you have to define

feat(menu): add nested menu functionality #5493

feat(menu): add nested menu functionality #5493

Conversation

crisbeto commented Jul 4, 2017

m98 commented Jul 5, 2017 • edited Loading

joejordanbrown commented Jul 5, 2017

jelbourn commented Jul 6, 2017

crisbeto commented Jul 6, 2017 • edited Loading

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jul 7, 2017

crisbeto commented Jul 7, 2017

jelbourn left a comment • edited Loading

Choose a reason for hiding this comment

Splaktar commented Jul 7, 2017

xuanshenbo commented Jul 9, 2017

kara left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jul 18, 2017

kara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto commented Jul 18, 2017 • edited Loading

kara left a comment

Choose a reason for hiding this comment

kara commented Jul 20, 2017

crisbeto commented Jul 20, 2017

kara commented Jul 20, 2017

kara commented Jul 20, 2017

crisbeto commented Jul 20, 2017

angular-automatic-lock-bot bot commented Sep 6, 2019

m98 commented Jul 5, 2017 •

edited

Loading

crisbeto commented Jul 6, 2017 •

edited

Loading

jelbourn left a comment •

edited

Loading

kara left a comment •

edited

Loading

crisbeto commented Jul 18, 2017 •

edited

Loading