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

amcdnl · 2017-09-17T17:53:29Z

Updates the Menu API with the following API modifications per #6677

close to closed
hover to hovered
onMenuOpen to menuOpened
onMenuClose to menuClosed

crisbeto · 2017-09-17T18:34:06Z

src/lib/menu/menu-trigger.ts

-    this._menuOpen ? this.onMenuOpen.emit() : this.onMenuClose.emit();
+
+    if (this._menuOpen) {
+      this.onMenuOpen.emit();


Rather than having to emit to multiple subjects, why not have something like:

@Output() onMenuOpen = new EventEmitter<void>(); @Output() onMenuOpened = this.onMenuOpen;

Thanks for the feedback! Done :)

jelbourn · 2017-09-19T04:53:22Z

src/lib/menu/menu-directive.ts

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


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

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

@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' }

jelbourn · 2017-09-19T04:54:16Z

src/lib/menu/menu-trigger.ts

+        if (this.menu.close) {
+          this.menu.close.emit();
+        }
+        this.menu.closed.emit();


Do you need this any more?

jelbourn · 2017-09-19T04:54:21Z

src/lib/menu/menu-trigger.ts

-      this.menu.close.emit();
+      if (this.menu.close) {
+        this.menu.close.emit();
+      }


crisbeto · 2017-09-19T19:40:37Z

src/lib/menu/menu-trigger.ts

@@ -203,7 +216,12 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
  openMenu(): void {
    if (!this._menuOpen) {
      this._createOverlay().attach(this._portal);
-      this._closeSubscription = this._menuClosingActions().subscribe(() => this.menu.close.emit());
+      this._closeSubscription = this._menuClosingActions().subscribe(() => {
+        if (this.menu.close) {


Why do we need this null check? The close emitter should always be there.

On the interface MdMenuPanel it declared a close EventEmitter. As part of this deprecation, I made that an optional interface and made closed the new interface. The implementation of this required I add a null check in those spots. If we are ok with removing that from the interface, we can remove all of this.

crisbeto · 2017-09-19T19:40:59Z

src/lib/menu/menu-trigger.ts

@@ -218,7 +236,11 @@ export class MdMenuTrigger implements AfterViewInit, OnDestroy {
      this._resetMenu();
      this._overlayRef.detach();
      this._closeSubscription.unsubscribe();
-      this.menu.close.emit();
+      if (this.menu.close) {


We shouldn't need this null check either.

See above comment.

crisbeto · 2017-09-23T08:02:00Z

src/lib/menu/menu-directive.ts

-    this._tabSubscription = this._keyManager.tabOut.subscribe(() => this.close.emit('keydown'));
+    this._tabSubscription = this._keyManager.tabOut.subscribe(() => {
+      this.close.emit('keydown');
+      this.closed.emit('keydown');


You shouldn't emit to both of these anymore. Since they're pointing to the same emitter, you'll end up emitting the same event twice.

amcdnl · 2017-09-24T14:53:18Z

Updated minus the new api question above.

jelbourn · 2017-09-25T22:06:47Z

Also just noticed- @kara @crisbeto why is there a hover event? Is that meant to be internal only? I would think downstream users would use mouseover etc. like normal

crisbeto · 2017-09-26T05:06:30Z

The hover event is internal, I added it to accommodate the nested menus.

jelbourn · 2017-09-26T17:15:46Z

Ack, @amcdnl can you change hover to just be internal?

amcdnl · 2017-09-30T18:48:50Z

@jelbourn - done.

jelbourn

LGTM

jelbourn · 2017-10-24T21:58:52Z

Caretaker note: should be completely backwards compatible, so if something fails there's a bug

amcdnl · 2017-10-26T19:13:59Z

Closing of favor of #8053

angular-automatic-lock-bot · 2019-09-07T11:03:17Z

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.}

chore(api): update menu api per angular#6677

a001ff0

amcdnl self-assigned this Sep 17, 2017

amcdnl requested a review from jelbourn September 17, 2017 17:53

amcdnl requested review from crisbeto and kara as code owners September 17, 2017 17:53

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 17, 2017

amcdnl removed request for kara and crisbeto September 17, 2017 17:53

crisbeto reviewed Sep 17, 2017

View reviewed changes

chore(nit): feedback tweaks

867bfab

jelbourn reviewed Sep 19, 2017

View reviewed changes

crisbeto reviewed Sep 19, 2017

View reviewed changes

jelbourn added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 22, 2017

crisbeto reviewed Sep 23, 2017

View reviewed changes

chore(nit): updates per feedback

68a4c4a

amcdnl added 3 commits September 30, 2017 11:24

Merge branch 'master' into menu-outputs-6677

22090ca

chore(test): fix tests

fd0ef24

chore(api): move hover internal

02dbd57

Merge branch 'master' into menu-outputs-6677

64f5f6c

jelbourn approved these changes Oct 24, 2017

View reviewed changes

jelbourn 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 labels Oct 24, 2017

amcdnl changed the title ~~chore(api): update menu api per #6677~~ fix(menu): make @Output names consistent #6677 Oct 25, 2017

amcdnl mentioned this pull request Oct 26, 2017

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

Merged

amcdnl closed this Oct 26, 2017

amcdnl deleted the menu-outputs-6677 branch October 26, 2017 19:14

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 7, 2019

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

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

Uh oh!

Conversation

amcdnl commented Sep 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Sep 24, 2017

Uh oh!

jelbourn commented Sep 25, 2017

Uh oh!

crisbeto commented Sep 26, 2017

Uh oh!

jelbourn commented Sep 26, 2017

Uh oh!

amcdnl commented Sep 30, 2017

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Oct 24, 2017

Uh oh!

amcdnl commented Oct 26, 2017

Uh oh!

angular-automatic-lock-bot bot commented Sep 7, 2019

Uh oh!

Uh oh!