Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix destory older pages when navigation from NavigationPage #1067

Merged
merged 6 commits into from
Jun 27, 2017
Merged

Fix destory older pages when navigation from NavigationPage #1067

merged 6 commits into from
Jun 27, 2017

Conversation

nuitsjp
Copy link
Contributor

@nuitsjp nuitsjp commented May 28, 2017

Please take a moment to fill out the following:

Fixes issue #1045 .

Changes proposed in this pull request:

When there are multiple pages in Navigation Stack, when navigation from NavigationPage, Destroy of removed page from Stack is not called.
Fixed this problem.

It is redoing of this PR #1051 .

@nuitsjp
Copy link
Contributor Author

nuitsjp commented May 28, 2017

I was wrong.
Here is the correct PR.


await DoNavigateAction(currentNavRoot, nextSegment, newRoot, parameters, async () =>
var replaseRoot = currentNavRoot.GetType() != nextPageType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

misspelling: replaceRoot

await ProcessNavigation(rootPage, segments, parameters, false, animated);
await DoNavigateAction(currentNavRoot, nextSegment, rootPage, parameters, async () =>
{
if (replaseRoot)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are not handling the scenario of when clearNavStack is false pages are pushed onto the stack. Right now, that scenario is broken. I should probably have a test for that :)

@nuitsjp
Copy link
Contributor Author

nuitsjp commented Jun 2, 2017

Thanks for the review.
It corresponds to the weekend. Maybe.

@nuitsjp
Copy link
Contributor Author

nuitsjp commented Jun 3, 2017

Fixed.
Please review.

Copy link
Member

@brianlagunas brianlagunas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there. It's looking good. Just one more scenario :)

{
var fromPage = currentPage.Navigation.NavigationStack.Last();
var nextPage = CreatePageFromSegment(nextSegment);
await ProcessNavigation(nextPage, segments, parameters, false, animated);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now when ClearNavigationStackOnNavigation is false and the current page is the same as the page being navigated to, you get a duplicate page. You need to also check to make sure the target page is not the same type as the current page. Similar to line 223

@nuitsjp
Copy link
Contributor Author

nuitsjp commented Jun 7, 2017

I understand and fixed it!
Please review.

@brianlagunas brianlagunas merged commit 161767a into PrismLibrary:master Jun 27, 2017
@nuitsjp nuitsjp deleted the fix-to-call-destory-when-poptorootasync branch June 30, 2017 20:54
@lock
Copy link

lock bot commented Jan 28, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants