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

Implement NavigateFromAsync method without recreating a page. #3182

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

niimima
Copy link
Contributor

@niimima niimima commented Jul 5, 2024

Description of Change

Adding NavigateFromAsync methods without recreating a page.

Bugs Fixed

API Changes

List all API changes here (or just put None),

Added:

  • Task INavigationService.NavigateFromAsync(string viewName, Uri route, INavigationParameters parameters);
  • Task INavigationServiceExtensions.NavigateFromAsync(this INavigationService navigationService, string viewName, Uri route)

Behavioral Changes

No changes. Only additions of features.

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard

@niimima
Copy link
Contributor Author

niimima commented Jul 5, 2024

I have a concern regarding the pull request I've created. When searching for a page using viewName, if it is not found, what would be the recommended behavior?
https://github.com/PrismLibrary/Prism/pull/3182/files#diff-8ae9340c7f10510f59a045175866a15bdb8ed35b541c8256cf73252dafadc343R350-R356

In the current implementation, pages specified by route appears directly under the window.

{
if (page is not null && ViewModelLocator.GetNavigationName(page) == viewName)
break;
page = page.GetParentPage();
Copy link
Contributor

Choose a reason for hiding this comment

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

Parent page? Shouldn't this be the page before the current page in the stack and if it is the root page then we get the parent page?

Copy link
Member

Choose a reason for hiding this comment

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

@AmrAlSayed0 is correct. While we do care about the parent in some scenarios, what we really need is the previous page in the navigation stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are still some issues remaining, but I have made corrections to use the NavigationStack as described in the following link.

// Find a page that matches the viewName.
var currentPage = GetCurrentPage();
var navigationPages = currentPage.Navigation.NavigationStack.ToList();
navigationPages.Reverse();
var foundPage = navigationPages.FirstOrDefault(page => ViewModelLocator.GetNavigationName(page) == viewName);
if (foundPage is null)
{
// Find a page from parents.
var page = currentPage;
while (page != null)
{
if (page is not null && ViewModelLocator.GetNavigationName(page) == viewName)
break;
page = page.GetParentPage();
}
currentPage = page;
}
else
{
// Insert RemovePageSegment.
var removePageCount = navigationPages.IndexOf(foundPage);
var tempQueue = new Queue<string>();
for (int i = 0; i < removePageCount; i++)
{
tempQueue.Enqueue(RemovePageSegment);
}
while(navigationSegments.Count > 0)
{
tempQueue.Enqueue(navigationSegments.Dequeue());
}
navigationSegments = tempQueue;
}

@brianlagunas
Copy link
Member

This PR is not performing the actions required of the issue. Per the issue

This API should then effectively do the same as ../../SomePage where the ../.. is replaced by going back until we found the View that was specified.

This means you must first find the page, while keeping track of the number of times we must navigate back until we reach this page, and then begin navigating forward from there.

Decisions must also be made as to whether this can be supported in both a modal and non-modal navigation stack. While also considering the page's parent such as a TabbedPage or FlyoutPage

@dansiegel dansiegel added this to the vNext milestone Jul 19, 2024
@niimima niimima requested a review from dansiegel as a code owner July 29, 2024 02:22
@niimima
Copy link
Contributor Author

niimima commented Jul 29, 2024

@brianlagunas

This PR is not performing the actions required of the issue. Per the issue

This API should then effectively do the same as ../../SomePage where the ../.. is replaced by going back until we found the View that was specified.

This means you must first find the page, while keeping track of the number of times we must navigate back until we reach this page, and then begin navigating forward from there.

Decisions must also be made as to whether this can be supported in both a modal and non-modal navigation stack. While also considering the page's parent such as a TabbedPage or FlyoutPage

Thank you for your feedback. Firstly, I am working on ensuring that the navigation works correctly with both TabbedPage and FlyoutPage. The FlyoutPage seems to be working well, but I need to confirm the specifications for the TabbedPage.

In the existing NavigateAsync method, you can use KnownNavigationParameters to either create or select a tab. For the NavigateFromAsync method, which approach would be more appropriate? Or, would it be better to dynamically switch between creating and selecting tabs based on the situation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Enhancement] NavigateFrom
4 participants