Skip to content

fix(menu): make @Output names consistent #6677 #7133

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

Closed
wants to merge 7 commits into from
Closed

fix(menu): make @Output names consistent #6677 #7133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a001ff0
chore(api): update menu api per #6677
amcdnl Sep 17, 2017
867bfab
chore(nit): feedback tweaks
amcdnl Sep 19, 2017
68a4c4a
chore(nit): updates per feedback
amcdnl Sep 24, 2017
22090ca
Merge branch 'master' into menu-outputs-6677
amcdnl Sep 30, 2017
fd0ef24
chore(test): fix tests
amcdnl Sep 30, 2017
02dbd57
chore(api): move hover internal
amcdnl Sep 30, 2017
64f5f6c
Merge branch 'master' into menu-outputs-6677
amcdnl Oct 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 16 additions & 10 deletions src/lib/menu/menu-directive.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,13 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {
}

/** Event emitted when the menu is closed. */
@Output() close = new EventEmitter<void | 'click' | 'keydown'>();
@Output() closed = new EventEmitter<void | 'click' | 'keydown'>();

/**
* Event emitted when the menu is closed.
* @deprecated Switch to `closed` instead
*/
@Output() close = this.closed;
Copy link
Member

Choose a reason for hiding this comment

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

Same question here as the select- should we just have one openedChange?
cc @kara @crisbeto

Copy link
Member

Choose a reason for hiding this comment

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

That's fine by me, just need to mark it as a breaking change or deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn - Ok, to verify we want to changeopen and close to openChange with a signature like:

export interface MdMenuEvent {
   opened: boolean;
   type: void | 'click' | 'keydown'
}


constructor(
private _elementRef: ElementRef,
Expand All @@ -154,42 +160,42 @@ export class MatMenu implements AfterContentInit, MatMenuPanel, OnDestroy {
this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.close.emit('keydown'));
}

ngOnDestroy() {
ngOnDestroy(): void {
this._tabSubscription.unsubscribe();
this.close.emit();
this.close.complete();
this.closed.emit();
this.closed.complete();
}

/** Stream that emits whenever the hovered menu item changes. */
hover(): Observable<MatMenuItem> {
_hovered(): Observable<MatMenuItem> {
if (this.items) {
return RxChain.from(this.items.changes)
.call(startWith, this.items)
.call(switchMap, (items: MatMenuItem[]) => merge(...items.map(item => item.hover)))
.call(switchMap, (items: MatMenuItem[]) => merge(...items.map(item => item._hovered)))
.result();
}

return RxChain.from(this._ngZone.onStable.asObservable())
.call(first)
.call(switchMap, () => this.hover())
.call(switchMap, () => this._hovered())
.result();
}

/** Handle a keyboard event from the menu, delegating to the appropriate action. */
_handleKeydown(event: KeyboardEvent) {
switch (event.keyCode) {
case ESCAPE:
this.close.emit('keydown');
this.closed.emit('keydown');
event.stopPropagation();
break;
case LEFT_ARROW:
if (this.parentMenu && this.direction === 'ltr') {
this.close.emit('keydown');
this.closed.emit('keydown');
}
break;
case RIGHT_ARROW:
if (this.parentMenu && this.direction === 'rtl') {
this.close.emit('keydown');
this.closed.emit('keydown');
}
break;
default:
Expand Down
6 changes: 3 additions & 3 deletions src/lib/menu/menu-item.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOptio
OnDestroy {

/** Stream that emits when the menu item is hovered. */
hover: Subject<MatMenuItem> = new Subject();
_hovered: Subject<MatMenuItem> = new Subject();

/** Whether the menu item is highlighted. */
_highlighted: boolean = false;
Expand All @@ -69,7 +69,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOptio
}

ngOnDestroy() {
this.hover.complete();
this._hovered.complete();
}

/** Used to set the `tabindex`. */
Expand All @@ -93,7 +93,7 @@ export class MatMenuItem extends _MatMenuItemMixinBase implements FocusableOptio
/** Emits to the hover stream. */
_emitHoverEvent() {
if (!this.disabled) {
this.hover.next(this);
this._hovered.next(this);
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/lib/menu/menu-panel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export interface MatMenuPanel {
yPosition: MenuPositionY;
overlapTrigger: boolean;
templateRef: TemplateRef<any>;
close: EventEmitter<void | 'click' | 'keydown'>;
closed: EventEmitter<void | 'click' | 'keydown'>;
parentMenu?: MatMenuPanel | undefined;
direction?: Direction;
focusFirstItem: () => void;
Expand Down
37 changes: 25 additions & 12 deletions src/lib/menu/menu-trigger.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,10 +107,22 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
@Input('matMenuTriggerFor') menu: MatMenuPanel;

/** Event emitted when the associated menu is opened. */
@Output() onMenuOpen = new EventEmitter<void>();
@Output() menuOpened = new EventEmitter<void>();

/**
* Event emitted when the associated menu is opened.
* @deprecated Switch to `menuOpened` instead
*/
@Output() onMenuOpen = this.menuOpened;

/** Event emitted when the associated menu is closed. */
@Output() onMenuClose = new EventEmitter<void>();
@Output() menuClosed = new EventEmitter<void>();

/**
* Event emitted when the associated menu is closed.
* @deprecated Switch to `menuClosed` instead
*/
@Output() onMenuClose = this.menuClosed;

constructor(private _overlay: Overlay,
private _element: ElementRef,
Expand All @@ -128,27 +140,27 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
ngAfterContentInit() {
this._checkMenu();

this.menu.close.subscribe(reason => {
this.menu.closed.subscribe(reason => {
this._destroyMenu();

// If a click closed the menu, we should close the entire chain of nested menus.
if (reason === 'click' && this._parentMenu) {
this._parentMenu.close.emit(reason);
this._parentMenu.closed.emit(reason);
}
});

if (this.triggersSubmenu()) {
// Subscribe to changes in the hovered item in order to toggle the panel.
this._hoverSubscription = filter
.call(this._parentMenu.hover(), active => active === this._menuItemInstance)
.call(this._parentMenu._hovered(), active => active === this._menuItemInstance)
.subscribe(() => {
this._openedByMouse = true;
this.openMenu();
});
}
}

ngOnDestroy() {
ngOnDestroy(): void {
if (this._overlayRef) {
this._overlayRef.dispose();
this._overlayRef = null;
Expand Down Expand Up @@ -182,7 +194,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
if (!this._menuOpen) {
this._createOverlay().attach(this._portal);
this._closeSubscription = this._menuClosingActions().subscribe(() => {
this.menu.close.emit();
this.menu.closed.emit();
});
this._initMenu();

Expand All @@ -194,7 +206,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {

/** Closes the menu. */
closeMenu(): void {
this.menu.close.emit();
this.menu.closed.emit();
}

/** Focuses the menu trigger. */
Expand Down Expand Up @@ -273,7 +285,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
// set state rather than toggle to support triggers sharing a menu
private _setIsMenuOpen(isOpen: boolean): void {
this._menuOpen = isOpen;
this._menuOpen ? this.onMenuOpen.emit() : this.onMenuClose.emit();
this._menuOpen ? this.menuOpened.emit() : this.menuClosed.emit();

if (this.triggersSubmenu()) {
this._menuItemInstance._highlighted = isOpen;
Expand All @@ -284,7 +296,7 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
* This method checks that a valid instance of MatMenu has been passed into
* matMenuTriggerFor. If not, an exception is thrown.
*/
private _checkMenu() {
private _checkMenu(): void {
if (!this.menu) {
throwMatMenuMissingError();
}
Expand Down Expand Up @@ -387,8 +399,9 @@ export class MatMenuTrigger implements AfterContentInit, OnDestroy {
/** 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 ? this._parentMenu.close : observableOf();
const hover = this._parentMenu ? RxChain.from(this._parentMenu.hover())
const parentClose = this._parentMenu ? this._parentMenu.closed : observableOf();

const hover = this._parentMenu ? RxChain.from(this._parentMenu._hovered())
.call(filter, active => active !== this._menuItemInstance)
.call(filter, () => this._menuOpen)
.result() : observableOf();
Expand Down
2 changes: 1 addition & 1 deletion src/lib/menu/menu.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
class="mat-menu-panel"
[ngClass]="_classList"
(keydown)="_handleKeydown($event)"
(click)="close.emit('click')"
(click)="closed.emit('click')"
[@transformMenu]="_panelAnimationState"
(@transformMenu.done)="_onAnimationDone($event)"
tabindex="-1"
Expand Down
8 changes: 4 additions & 4 deletions src/lib/menu/menu.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ describe('MatMenu', () => {
const emitCallback = jasmine.createSpy('emit callback');
const completeCallback = jasmine.createSpy('complete callback');

fixture.componentInstance.menu.close.subscribe(emitCallback, null, completeCallback);
fixture.componentInstance.menu.closed.subscribe(emitCallback, null, completeCallback);
fixture.destroy();

expect(emitCallback).toHaveBeenCalledWith(undefined);
Expand Down Expand Up @@ -625,7 +625,7 @@ describe('MatMenu', () => {
fixture.detectChanges();

const spy = jasmine.createSpy('hover spy');
const subscription = instance.rootMenu.hover().subscribe(spy);
const subscription = instance.rootMenu._hovered().subscribe(spy);
const menuItems = overlay.querySelectorAll('[mat-menu-item]');

dispatchMouseEvent(menuItems[0], 'mouseenter');
Expand Down Expand Up @@ -1112,7 +1112,7 @@ describe('MatMenu default overrides', () => {
@Component({
template: `
<button [matMenuTriggerFor]="menu" #triggerEl>Toggle menu</button>
<mat-menu class="custom-one custom-two" #menu="matMenu" (close)="closeCallback($event)">
<mat-menu class="custom-one custom-two" #menu="matMenu" (closed)="closeCallback($event)">
<button mat-menu-item> Item </button>
<button mat-menu-item disabled> Disabled </button>
<button mat-menu-item>
Expand Down Expand Up @@ -1181,7 +1181,7 @@ class CustomMenuPanel implements MatMenuPanel {
parentMenu: MatMenuPanel;

@ViewChild(TemplateRef) templateRef: TemplateRef<any>;
@Output() close = new EventEmitter<void | 'click' | 'keydown'>();
@Output() closed = new EventEmitter<void | 'click' | 'keydown'>();
focusFirstItem = () => {};
resetActiveItem = () => {};
setPositionClasses = () => {};
Expand Down