feat(overlay): support custom navigation event signals for auto dispose

[dscheerens]

Currently when overlays are opened with the disposeOnNavigation option set to true they only close automatically when the user uses the browser history back / forward functions. However, when the @angular/router module is used, clicking an element with a [navigationLink] directive will not cause the overlay to be closed. This can be explained by the fact that this behavior is implemented using the Location service from @angular/common. Unfortunately that service only emits a signal whenever a popstate event occurs. This feature replaces the Location dependency for a more abstract NavigationSignal, allowing consumers of the overlay module to specify their own custom stream of navigation events, e.g. based on the Router.event stream. When no such stream is specified (via the NAVIGATION_SIGNAL injection token) then Location is used as default.

[jelbourn]

Thanks for the PR!

My main question here is whether providing this NavigationSignal is actually less work than setting up that same stream in your app and calling an Overlay API to close one (or all) overlays.

@crisbeto thoughts?

[crisbeto]

I've got the same concern as @jelbourn: it seems like providing the signal would be a bit more code than subscribing to the router events manually and closing the overlay. Furthmore this could be a bit less flexible since it doesn't allow for operators to be piped in. An alternate approach could be to allow consumers to pass in an Observable which will trigger a detach call when it emits, but even in that case we'd only be calling subscribe for the consumer.

[dscheerens]

Furthmore this could be a bit less flexible since it doesn't allow for operators to be piped in. An alternate approach could be to allow consumers to pass in an Observable which will trigger a detach call when it emits, but even in that case we'd only be calling subscribe for the consumer.

Yeah I agree about the flexibility part. Initially thought of making the NavigationSignal an Observable, but that would mean breaking the public API (slightly)

[dscheerens]

My main question here is whether providing this NavigationSignal is actually less work than setting up that same stream in your app and calling an Overlay API to close one (or all) overlays.

You are right about that :) That is how it is done currently in many applications.

Though, for me this felt like a cleaner (more declarative) solution + it broadens the usefulness of the disposeOnNavigation option.

[dscheerens]

Just found out about the following change in Angular 8.0:

feat(common): add ability to track all location changes (#30055)

@jelbourn @crisbeto: would you be open for a change that makes use of this feature (Location.onUrlChange), once there would be a way to unsubscribe from these events (which is currently not possible).

[jelbourn]

Using onUrlChange sounds reasonable; @jasonaden what's the story on not being able to unsubscribe?

[crisbeto]

Closing since it looks like there hasn't been any activity in a while and the need for the change was unclear.

[angular-automatic-lock-bot[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.}