Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(menu): add nested menu functionality #5493

Merged
merged 6 commits into from
Jul 20, 2017
Merged

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jul 4, 2017

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.

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

m98 commented Jul 5, 2017

  1. 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)
    screenshot from 2017-07-05 13-43-43

  2. 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
Copy link
Contributor

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.

image

This page here documents it.

@jelbourn
Copy link
Member

jelbourn commented Jul 6, 2017

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
Copy link
Member Author

crisbeto commented Jul 6, 2017

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

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a function to avoid the compiled getter boilerplate?

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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be an internal api?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this a function?


if (!this.menu.overlapTrigger) {
if (this.triggersSubmenu) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jelbourn jelbourn requested a review from kara July 6, 2017 22:08
@crisbeto
Copy link
Member Author

crisbeto commented Jul 7, 2017

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
Copy link
Member Author

crisbeto commented Jul 7, 2017

Addressed the feedback @jelbourn.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Splaktar
Copy link
Member

Splaktar commented Jul 7, 2017

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
Copy link

wrt adding an indicator, could be related to #5067

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, a couple comments.

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

  1. 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?

  2. 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?


### Nested menu

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kara kara assigned crisbeto and unassigned kara Jul 18, 2017
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
Copy link
Member Author

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?

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

/** 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

crisbeto commented Jul 18, 2017

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

Copy link
Contributor

@kara kara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kara 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
Copy link
Contributor

kara commented Jul 20, 2017

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

@crisbeto
Copy link
Member Author

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
Copy link
Contributor

kara commented Jul 20, 2017

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

@kara
Copy link
Contributor

kara commented Jul 20, 2017

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

@kara kara removed the action: merge The PR is ready for merge by the caretaker label Jul 20, 2017
@crisbeto
Copy link
Member Author

Done.

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jul 20, 2017
@kara 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 kara merged commit 1e0c1fc into angular:master Jul 20, 2017
@jelbourn jelbourn mentioned this pull request Jul 21, 2017
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: Tiered option for Menu Component
8 participants