fix(menu): update to use overlay backdrop

[kara]

Closes#1360, #1499, #1506 .

r: @jelbourn

[jelbourn]

Unit test?

[kara]

There's an e2e that covers this already, but I can add a unit too if you think it will help coverage?

[fxck]

Shouldn't it be

this._overlayRef.backdropClick().take(1).subscribe(()=>this.closeMenu());

to unsubscribe the observer since the backdrop element will be removed from DOM anyway? see https://jsfiddle.net/chy8gLjs/ vs. https://jsfiddle.net/j4p7gq1v/

[jelbourn]

Since the backdrop doesn't have anything to do with positioning, I'd do this as a unit tests. We generally want to prefer capturing behaviors with unit tests because they are so much faster.

@fxck I would use .first(), but yeah that should be the the case.

[fxck]

Same thing. Anyway I think there's couple more of these potential leaks across the repo. Either not using take/first when it's supposed to happen only once or not unsubscribing on destroy, when it's actually supposed to listen on changes until the component is destroyed. I'll create an issue if I get couple of minutes.

[jelbourn]

LGTM

[jelbourn]

Looks like you might have to .trim() your textContent for mobile safari

[kara]

@jelbourn Finally passing.

[angular-automatic-lock-bot]

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