Conversation
|
|
||
| [Test] | ||
| [Category(UITestCategories.Navigation)] | ||
| public void VerifyNavigationEventArgs() |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the navigation event argument classes and their corresponding usages by adding a new NavigationType parameter and exposing additional properties.
- Updated NavigatedToEventArgs and NavigatingFromEventArgs with a NavigationType parameter and made PreviousPage public.
- Propagated the changes across NavigationPage, NavigationPage.Legacy, ModalNavigationManager, FlyoutPage, NavigationModel, and MultiPage to pass appropriate NavigationType values during navigation events.
Reviewed Changes
Copilot reviewed 27 out of 27 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Controls/src/Core/Page/NavigatingFromEventArgs.cs | Added NavigationType property and updated constructor to accept destination page and navigation type |
| src/Controls/src/Core/Page/NavigatedToEventArgs.cs | Updated constructor signature to include NavigationType and made PreviousPage public |
| src/Controls/src/Core/NavigationPage/NavigationPage.cs | Updated calls to SendNavigated and SendNavigating with NavigationType parameters |
| src/Controls/src/Core/NavigationPage/NavigationPage.Legacy.cs | Updated legacy navigation methods to use the new NavigationType parameters |
| src/Controls/src/Core/Platform/ModalNavigationManager/ModalNavigationManager.cs | Updated modal navigation lifecycle calls with NavigationType parameters |
| src/Controls/src/Core/Page/NavigatedFromEventArgs.cs | Changed Visibility of NavigationType enum and related properties from internal to public |
| src/Controls/src/Core/FlyoutPage/FlyoutPage.cs | Updated flyout navigation event calls with a NavigationType to indicate PageSwap events |
| src/Controls/src/Core/NavigationModel.cs | Updated modal navigation events with explicit NavigationType parameters |
| src/Controls/src/Core/MultiPage.cs | Amended current page change events to pass NavigationType for PageSwap events |
PureWeen
left a comment
There was a problem hiding this comment.
We should discuss this more as a team before adding these APIs
| }, | ||
| () => | ||
| { | ||
| // TODO this is the wrong navigation type |
There was a problem hiding this comment.
This was passing the wrong navigation type parameter. If we not decide to update the navigation EventArgs with the properties included in this PR, the changes to fix this should be included in another one.
|
One area I'm curious about with this one is obsoleting and making sure we aren't ending up with duplicate enums For example, we already have |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
I am adding a do not merge label as @PureWeen said things about discussions. Not sure if that happened, but just in case... |
In NavigatedFromEventArgs, we have been using the NavigationType enum for defining navigation types. (Reference). This PR introduces two key improvements:
Compared to using NavigationRequestType everywhere (Reference), this approach results in less disruptive changes, maintaining compatibility while refining consistency. However, both enums are nearly identical, except for certain differences like PageSwap value. Since we are already introducing breaking changes in .NET 10, this is an opportunity to standardize the navigation enum by maintaining a single, unified type for all cases where navigation type needs to be indicated. |
Not yet, we have to review it. |
|
Discuss naming "PageSwap" and the existence of other types like ShellNavigationSource |
|
Based on recently added tests, I've identified a platform discrepancy when using FlyoutPage. Specifically:
This inconsistency leads to different navigation behavior across platforms for the same layout construct, and failing tests. We must align this behavior so that navigation triggered by changing the FlyoutItem consistently uses NavigationType.Replace, just like it does when switching tabs with TabbedPage. Is that the expected behavior @PureWeen? That would better reflect the nature of layout-level navigation and ensure reliable cross-platform behavior. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| _detail?.SendNavigatingFrom(new NavigatingFromEventArgs()); | ||
|
|
||
| // Get the actual pages for navigation events (unwrap NavigationPages) | ||
| var destinationPage = |
|
Finally, can't wait to get it on net 10 GA so I can use Shell without breaking MVVM nor causing memory leak because of extensive usage of Rx.NET 🙏🏼 |

Description of Change
Changes in navigation events parameters. This PR apply the following changes:
NavigatedToEventArgs
NavigatingFromEventArgs
Issues Fixed
Fixes #21814