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.

Sign up for GitHub

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

Jump to bottom

feat(menu): support lazy rendering and passing in context data #9271

Merged
merged 1 commit into from
Jan 25, 2018
Merged

feat(menu): support lazy rendering and passing in context data #9271

merged 1 commit into from
Jan 25, 2018

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Jan 7, 2018

  • Introduces the matMenuContent directive that allows for menu content to be rendered lazily.
  • Adds the matMenuTriggerData input to the MatMenuTrigger that allows for contextual data to be passed in to the lazily-rendered menu panel. This allows for the menu instance to be re-used between triggers.

Fixes #9251.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 7, 2018
@jelbourn jelbourn added the target: minor This PR is targeted for the next minor release label Jan 10, 2018
jelbourn
jelbourn reviewed Jan 11, 2018
View reviewed changes
src/lib/menu/menu-content.ts Outdated
const element: HTMLElement = this._template.elementRef.nativeElement;

// Since the menu might be attached to a different DOM node, we have to re-insert it every time.
element.parentNode!.insertBefore(this._outlet.hostDomElement, element);
Copy link
Member

Choose a reason for hiding this comment

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

I don't quite follow why this is necessary

Copy link
Member Author

@crisbeto crisbeto Jan 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It's because we can have the menu be opened by different triggers which in turn have their own OverlayRef with a different pane. If we didn't do this, the element could end up in limbo if you opened it using two different triggers.

Copy link
Member

Choose a reason for hiding this comment

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

Add that explanation as a comment?

src/lib/menu/menu-content.ts
import {DOCUMENT} from '@angular/common';

/**
* Menu content that will be rendered lazily once the menu is opened.
Copy link
Member

Choose a reason for hiding this comment

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

Would an end user need to ever interact with this class (i.e. should it be docs-private)?

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, because we support providing your own menu instance that matches the MatMenuPanel interface.

src/lib/menu/menu.md
with the `matMenuContent` attribute:

```html
<mat-menu #appMenu="matMenu">
Copy link
Member

Choose a reason for hiding this comment

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

What do you think of avoiding the extra matMenuContent by letting people add matMenu to an ng-template element? Something like

<ng-template matMenu>
  ...
</ng-template>

The selector for the menu would change to mat-menu, ng-template[matMenu]. The menu would then optionally inject TemplateRef and go through the extra steps if one is present.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's what I was going for initially, but Angular throws an error when you have a component on an ng-template. The alternative would be to use a directive, but we can't do that either, because we need to wrap the menu content in a div to add the proper styling and for the animation to work.

jelbourn
jelbourn approved these changes Jan 11, 2018
View reviewed changes
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.

LGTM, just minor comments. Add merge-ready when ready

src/lib/menu/menu-content.ts Outdated
const element: HTMLElement = this._template.elementRef.nativeElement;

// Since the menu might be attached to a different DOM node, we have to re-insert it every time.
element.parentNode!.insertBefore(this._outlet.hostDomElement, element);
Copy link
Member

Choose a reason for hiding this comment

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

Add that explanation as a comment?

src/cdk/portal/dom-portal-outlet.ts Outdated
@@ -22,7 +22,8 @@ import {BasePortalOutlet, ComponentPortal, TemplatePortal} from './portal';
*/
export class DomPortalOutlet extends BasePortalOutlet {
constructor(
private _hostDomElement: Element,
/** Element into which the content is projected. */
public hostDomElement: Element,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe rename this to outletElement? I think the "host" is leftover from the old class name

@crisbeto crisbeto added the action: merge The PR is ready for merge by the caretaker label Jan 11, 2018
@jelbourn
Copy link
Member

@crisbeto passes Google presubmit, just needs rebase

@crisbeto
Copy link
Member Author

Rebased.

@jelbourn
Copy link
Member

@crisbeto failing on Travis now

@crisbeto
feat(menu): support lazy rendering and passing in context data
192be85 
* Introduces the `matMenuContent` directive that allows for menu content to be rendered lazily.
* Adds the `matMenuTriggerData` input to the `MatMenuTrigger` that allows for contextual data to be passed in to the lazily-rendered menu panel. This allows for the menu instance to be re-used between triggers.

Fixes angular#9251.
@crisbeto
Copy link
Member Author

Fixed the test failure.

@jelbourn jelbourn merged commit 9fed87c into angular:master Jan 25, 2018
@crisbeto crisbeto mentioned this pull request Apr 13, 2018
Open
@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 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@kara kara Awaiting requested review from kara

Assignees

@jelbourn jelbourn

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

mat-menu is rendering eagerly
3 participants
@crisbeto @jelbourn @googlebot