fix(menu): multiple close events for a single close #6961

ppham27 · 2017-09-10T04:06:56Z

Don't emit a closed event when another event will be emitted. Previously, if one clicked on a menu item, one would get two events: undefined and click in that order. One would see similar behavior for keydown or clicking the backdrop. Unit tests were updated to prevent a regression.

Don't emit a closed event when another event will be emitted. Previously, if one clicked on a menu item, one would get two events: `undefined` and `click` in that order. One would see similar behavior for `keydown` or clicking the backdrop. Unit tests were updated to prevent a regression.

crisbeto · 2017-09-10T06:48:25Z

src/lib/menu/menu-directive.ts

@@ -139,7 +139,7 @@ export class MdMenu implements AfterContentInit, MdMenuPanel, OnDestroy {
  }

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


The idea with this one was to emit the reason only if it's user-generated. Given that there is no special behavior for destroy, can you revert it back to void | 'click' | 'keydown'?

Done. I was also wondering if it's a bug that destroy will emit a close event when the menu is already closed?

It's there to notify any child menus to close.

But child menus won't be open unless the menu itself is open? Anyway, I'm more than happy to leave it as it is, as it doesn't really affect any use cases that I can think of.

In most cases the children won't be open without a parent, but it's possible for the parent to be destroyed while the children are still open.

crisbeto · 2017-09-10T06:48:59Z

src/lib/menu/menu-trigger.ts

+   * Gives the option of setting emitEvent to false to avoid emitting an event
+   * when an event will already be emitted.
+   */
+  private _closeMenu(emitEvent: boolean) {


Rather than having the emitEvent parameter, you can remove the this.menu.close.emit call from here and move it only to closeMenu. Also change the doc string to something along the lines of "Closes the menu and does the necessary cleanup".

Done. Should I only emit the event when the menu is already open, though?

crisbeto · 2017-09-10T06:54:23Z

src/lib/menu/menu.spec.ts

@@ -485,6 +485,7 @@ describe('MdMenu', () => {
      fixture = TestBed.createComponent(SimpleMenu);
      fixture.detectChanges();
      fixture.componentInstance.trigger.openMenu();
+      fixture.componentInstance.closeCallback.calls.reset();


Why is this necessary? The callbacks should be reset after each test, because the component is re-compiled.

They were necessary when because Observable.of(null) was triggering a close event. Replaced with Observable.empty()

Observable.of(null) will emit null immediately. Observable.of() won't.

Are you saying that I should change it to Observable.of() instead of Observable.empty()? To me, Observable.empty() is more readable and makes the intent more clear.

But I don't mind change it to Observable.of() since the behavior would be the same as Observable.empty().

Observable.empty() is definitely valid in this case. I just want to avoid bringing in an extra symbol from RxJS.

Changed to Observable.of()

crisbeto · 2017-09-10T06:54:59Z

src/lib/menu/menu.spec.ts

+      expect(fixture.componentInstance.closeCallback).toHaveBeenCalledTimes(1);
+    });
+
+    it('should emit an event when escaped', () => {


"should emit an event when pressing ESCAPE".

crisbeto · 2017-09-10T06:56:26Z

src/lib/menu/menu.spec.ts

@@ -989,11 +1004,18 @@ describe('MdMenu', () => {

      expect(menus.length).toBe(3, 'Expected three open menus');

+      instance.rootCloseCallback.calls.reset();


These reset calls shouldn't be necessary.

They were necessary when because Observable.of(null) was triggering a close event. Replaced with Observable.empty()

crisbeto · 2017-09-10T20:50:25Z

src/lib/menu/menu-trigger.ts

+  /**
+   * Closes the menu and does the necessary cleanup.
+   */
+  private _closeMenu() {


One last nitpick: can you rename this to _destroyMenu to avoid confusion with closeMenu?

_closeMenu -> _destroyMenu

crisbeto

LGTM, the two CI failures will be resolved by #6956.

jelbourn

LGTM

This reverts commit 1cccd4b.

* Revert "feat(datepicker): Add Moment.js adapter (#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (#6961)" This reverts commit 1cccd4b.

This reverts commit 1cccd4b.

* Revert "Revert "fix(menu): multiple close events for a single close" (#7036)" This reverts commit dcfe515. * Revert "feat(datepicker): Add Moment.js adapter (#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (#6961)" This reverts commit 1cccd4b. * Revert "fix(menu): nested trigger staying highlighted after click (#6853)" This reverts commit 04bf3d1. * Revert "feat(viewport-ruler): add common window resize handler (#6680)" This reverts commit 881630f.

@crisbeto

* fix(menu): multiple close events for a single close Don't emit a closed event when another event will be emitted. Previously, if one clicked on a menu item, one would get two events: `undefined` and `click` in that order. One would see similar behavior for `keydown` or clicking the backdrop. Unit tests were updated to prevent a regression. * Responding to review comments from @crisbeto * Observable.empty() -> Observable.of() _closeMenu -> _destroyMenu

…#7036) * Revert "feat(datepicker): Add Moment.js adapter (angular#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (angular#6961)" This reverts commit 1cccd4b.

…lar#7054) * Revert "Revert "fix(menu): multiple close events for a single close" (angular#7036)" This reverts commit dcfe515. * Revert "feat(datepicker): Add Moment.js adapter (angular#6860)" This reverts commit 9545427. * Revert "fix(menu): multiple close events for a single close (angular#6961)" This reverts commit 1cccd4b. * Revert "fix(menu): nested trigger staying highlighted after click (angular#6853)" This reverts commit 04bf3d1. * Revert "feat(viewport-ruler): add common window resize handler (angular#6680)" This reverts commit 881630f.

angular-automatic-lock-bot · 2019-09-07T01:03:38Z

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

ppham27 requested review from crisbeto and kara as code owners September 10, 2017 04:06

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

crisbeto reviewed Sep 10, 2017

View reviewed changes

Responding to review comments from @crisbeto

50d3214

ppham27 requested review from devversion and jelbourn as code owners September 10, 2017 20:20

crisbeto reviewed Sep 10, 2017

View reviewed changes

Observable.empty() -> Observable.of()

a396846

_closeMenu -> _destroyMenu

crisbeto approved these changes Sep 10, 2017

View reviewed changes

crisbeto added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Sep 10, 2017

jelbourn approved these changes Sep 11, 2017

View reviewed changes

mmalerba merged commit 1cccd4b into angular:master Sep 12, 2017

mmalerba added a commit that referenced this pull request Sep 13, 2017

Revert "fix(menu): multiple close events for a single close (#6961)"

72da552

This reverts commit 1cccd4b.

mmalerba mentioned this pull request Sep 13, 2017

Revert "fix(menu): multiple close events for a single close" #7036

Merged

mmalerba added a commit that referenced this pull request Sep 13, 2017

Revert "fix(menu): multiple close events for a single close (#6961)"

4011b40

This reverts commit 1cccd4b.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(menu): multiple close events for a single close #6961

fix(menu): multiple close events for a single close #6961

ppham27 commented Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto Sep 10, 2017

ppham27 Sep 10, 2017

crisbeto left a comment

jelbourn left a comment

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

		@@ -989,11 +1004,18 @@ describe('MdMenu', () => {

		expect(menus.length).toBe(3, 'Expected three open menus');

		instance.rootCloseCallback.calls.reset();

fix(menu): multiple close events for a single close #6961

fix(menu): multiple close events for a single close #6961

Conversation

ppham27 commented Sep 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisbeto left a comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

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