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

Allow menu-trigger to take a menu interface. #1564

Merged
merged 1 commit into from
Nov 1, 2016

Conversation

trshafer
Copy link
Contributor

@trshafer trshafer commented Oct 21, 2016

This should enable menus to be more easily extended for custom menu implementations.

Addresses #1560.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Oct 21, 2016
@trshafer
Copy link
Contributor Author

I went back and forth on whether or not to make it an abstract class with many of the implementation details on the abstract class. I went with the interface because it's less invasive to the consumer than an abstract class.

@trshafer
Copy link
Contributor Author

Force pushed to address the conflict in merge of #1534.

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.

It would be good to also have a unit test that binds a dummy impl of MdMenuPanel and assert that a basic action works.

templateRef: TemplateRef<any>;
close: EventEmitter<void>;
_focusFirstItem: () => void;
_setClickCatcher: (bool: boolean) => void;
Copy link
Member

Choose a reason for hiding this comment

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

If these two are going to be part of this interface, they'll should lose the underscore (which implies "internal api only")

import {EventEmitter, TemplateRef} from '@angular/core';
import {MenuPositionX, MenuPositionY} from './menu-positions';

export interface MdMenuInterface {
Copy link
Member

Choose a reason for hiding this comment

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

Let's call this MdMenuPanel
(generally trying to avoid using the word "interface" in the names of interfaces)

@trshafer
Copy link
Contributor Author

Hey @jelbourn, I have made the requested updates. Good suggestions. The test really helps demonstrate the creation of custom menus.

@jelbourn
Copy link
Member

LGTM aside from a couple last comments. @kara should also take a look

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, aside from the one comment. Looks like this needs to be rebased as well

positionY: MenuPositionY;
templateRef: TemplateRef<any>;
close: EventEmitter<void>;
_focusFirstItem: () => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @jelbourn that underscore should be removed if possible

This should enable menus to be more easily extended for custom menu implementations.
@trshafer
Copy link
Contributor Author

Hey @kara, thanks for the review. I have removed the underscore from the interface, rebased, squashed my commits, and updated the commit message style.

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 and removed pr: needs rebase labels Oct 31, 2016
@hansl hansl merged commit 96d196a into angular:master Nov 1, 2016
@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.

5 participants