fix(dialog): close all dialogs on popstate/hashchange #2742

crisbeto · 2017-01-21T12:29:53Z

Closes all of the open dialogs when the user goes forwards/backwards in history.

devversion · 2017-01-21T12:58:25Z

src/lib/dialog/dialog.ts

-      // through it. We loop through all of them and call close without assuming that
-      // they'll be removed from the list instantaneously.
-      this._openDialogs[i].close();
+    if (i > 0) {


Is this check necessary?

i-- should return zero at beginning and the while would not run at all. And i should be always an integer from [0; X] .

Not really. I'll remove it.

jelbourn · 2017-01-23T20:05:58Z

LGTM

@vicb can you confirm that this is a reasonable thing to do?

vicb · 2017-01-23T20:10:30Z

src/lib/dialog/dialog.ts

+    // Close all of the dialogs when the user goes forwards/backwards in history or when the
+    // location hash changes. Note that this usually doesn't include clicking on links (unless
+    // the user is using the `HashLocationStrategy`).
+    _location.subscribe(() => this.closeAll());


you probably want to unsubscribe in ngOnDestroy

This is inside of a service.

vicb · 2017-01-23T20:14:18Z

LGTM with un-subscription + test

jelbourn · 2017-01-23T23:53:35Z

Dialog actually isn't a singleton; in apps with lazy-loading, you necessarily have multiple instances of MdDialog, which means that we need to delegate to the ancestor dialog if there is one.

crisbeto · 2017-01-24T18:15:02Z

Done.

jelbourn · 2017-01-24T21:23:12Z

LGTM

kara · 2017-02-09T19:00:31Z

@crisbeto Rebase?

andrewseguin · 2017-02-17T20:13:19Z

Can you rebase once again :)

mmalerba · 2017-02-23T23:22:44Z

please rebase

crisbeto · 2017-03-16T20:47:12Z

Sorry for the delay on this one, I must've missed the notification.

mmalerba · 2017-03-17T17:36:15Z

There are a number of tests failing in google because there is no provider for Location. Is this something that can be solved by importing somew module in MdDialogModule? or do we just need to make all of these failing tests provide SpyLocation?

mmalerba · 2017-03-17T18:00:13Z

@vicb Any advice on how to handle that fact that using MdDialog now requires Location to be provided in tests? It would be nice if we didn't force everyone to add it to their tests manually

mmalerba · 2017-04-25T17:37:22Z

@crisbeto The way this is currently set up I have to provide SpyLocation in everyone's unit tests to get them to not fail, could you make the Location optional and just skip the close on fwd/back if its not provided?

Closes all of the open dialogs when the user goes forwards/backwards in history. Fixes angular#2601.

angular-automatic-lock-bot · 2019-09-06T12:01:32Z

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

crisbeto added the pr: needs review label Jan 21, 2017

crisbeto assigned jelbourn Jan 21, 2017

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

devversion reviewed Jan 21, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Jan 23, 2017

vicb reviewed Jan 23, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Jan 24, 2017

crisbeto force-pushed the 2601/dialog-pop-state branch from 5338648 to a9ef8f1 Compare January 31, 2017 21:21

crisbeto force-pushed the 2601/dialog-pop-state branch from a9ef8f1 to 3b128a3 Compare February 7, 2017 22:06

kara added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 9, 2017

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Feb 9, 2017

andrewseguin added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 17, 2017

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Feb 17, 2017

crisbeto force-pushed the 2601/dialog-pop-state branch from d032601 to 1b78613 Compare February 21, 2017 18:45

mmalerba added pr: needs rebase and removed action: merge The PR is ready for merge by the caretaker labels Feb 22, 2017

crisbeto force-pushed the 2601/dialog-pop-state branch from 1b78613 to ccbfedc Compare March 16, 2017 20:36

crisbeto added action: merge The PR is ready for merge by the caretaker and removed pr: needs rebase labels Mar 16, 2017

crisbeto force-pushed the 2601/dialog-pop-state branch from ccbfedc to eca455c Compare March 23, 2017 22:07

jelbourn added the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 11, 2017

crisbeto force-pushed the 2601/dialog-pop-state branch 2 times, most recently from 4abed15 to 6811780 Compare April 24, 2017 17:40

mmalerba added in progress This issue is currently in progress and removed action: merge The PR is ready for merge by the caretaker labels Apr 25, 2017

mmalerba assigned crisbeto Apr 25, 2017

fix(dialog): close all dialogs on popstate/hashchange

278d635

Closes all of the open dialogs when the user goes forwards/backwards in history. Fixes angular#2601.

crisbeto force-pushed the 2601/dialog-pop-state branch from 6811780 to 278d635 Compare April 25, 2017 17:43

crisbeto added action: merge The PR is ready for merge by the caretaker and removed in progress This issue is currently in progress labels Apr 25, 2017

mmalerba removed the presubmit failures This PR has failures in Google's internal presubmit process and cannot be immediately merged label Apr 25, 2017

mmalerba merged commit 85bc3a6 into angular:master Apr 25, 2017

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(dialog): close all dialogs on popstate/hashchange #2742

fix(dialog): close all dialogs on popstate/hashchange #2742

crisbeto commented Jan 21, 2017

devversion Jan 21, 2017

crisbeto Jan 21, 2017

jelbourn commented Jan 23, 2017

vicb Jan 23, 2017

crisbeto Jan 23, 2017 •

edited

Loading

vicb commented Jan 23, 2017

jelbourn commented Jan 23, 2017

crisbeto commented Jan 24, 2017

jelbourn commented Jan 24, 2017

kara commented Feb 9, 2017

andrewseguin commented Feb 17, 2017

mmalerba commented Feb 23, 2017

crisbeto commented Mar 16, 2017

mmalerba commented Mar 17, 2017 •

edited by jelbourn

Loading

mmalerba commented Mar 17, 2017

mmalerba commented Apr 25, 2017

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

fix(dialog): close all dialogs on popstate/hashchange #2742

fix(dialog): close all dialogs on popstate/hashchange #2742

Conversation

crisbeto commented Jan 21, 2017

devversion Jan 21, 2017

Choose a reason for hiding this comment

crisbeto Jan 21, 2017

Choose a reason for hiding this comment

jelbourn commented Jan 23, 2017

vicb Jan 23, 2017

Choose a reason for hiding this comment

crisbeto Jan 23, 2017 • edited Loading

Choose a reason for hiding this comment

vicb commented Jan 23, 2017

jelbourn commented Jan 23, 2017

crisbeto commented Jan 24, 2017

jelbourn commented Jan 24, 2017

kara commented Feb 9, 2017

andrewseguin commented Feb 17, 2017

mmalerba commented Feb 23, 2017

crisbeto commented Mar 16, 2017

mmalerba commented Mar 17, 2017 • edited by jelbourn Loading

mmalerba commented Mar 17, 2017

mmalerba commented Apr 25, 2017

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

crisbeto Jan 23, 2017 •

edited

Loading

mmalerba commented Mar 17, 2017 •

edited by jelbourn

Loading