Uh oh!
There was an error while loading. Please reload this page.
Allow menu-trigger to take a menu interface. - #1564
Conversation
trshafer
commented
Oct 21, 2016
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
commented
Oct 21, 2016
Force pushed to address the conflict in merge of #1534. |
jelbourn
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
Let's call this MdMenuPanel
(generally trying to avoid using the word "interface" in the names of interfaces)
trshafer
commented
Oct 24, 2016
Hey @jelbourn, I have made the requested updates. Good suggestions. The test really helps demonstrate the creation of custom menus. |
jelbourn
commented
Oct 24, 2016
LGTM aside from a couple last comments. @kara should also take a look |
kara
left a comment
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Agree with @jelbourn that underscore should be removed if possible
This should enable menus to be more easily extended for custom menu implementations.
trshafer
commented
Oct 31, 2016
Hey @kara, thanks for the review. I have removed the underscore from the interface, rebased, squashed my commits, and updated the commit message style. |
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This should enable menus to be more easily extended for custom menu implementations.
Addresses #1560.