-
Notifications
You must be signed in to change notification settings - Fork 1.9k
30985:Fix FlyoutPage Navigating args to not reach down into the NavigationPage #31232
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
Conversation
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR simplifies the navigation event handling logic in FlyoutPage by removing NavigationPage unwrapping and streamlining the code. The change ensures that navigation events (SendNavigatingFrom, SendNavigatedFrom, and SendNavigatedTo) are called directly on the actual Page objects rather than attempting to unwrap NavigationPages to get their current pages.
Key changes:
- Removes complex NavigationPage unwrapping logic that was extracting CurrentPage from NavigationPages
- Simplifies null checking using the null-conditional operator
- Updates navigation event arguments to pass the actual Page objects directly
PureWeen
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SuthiYuvaraj can you add some unit tests for this change?
There should be a pretty large set of unit tests (not UI tests) already testing this feature
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
736f6ea to
6d448f4
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@SuthiYuvaraj it looks like the test failure might be real? Or we need to rebase this? |
|
@PureWeen, This failure is due to a flaky test case (PullToRefreshWorksWhenEnabled) and is not related to my changes. We can try rerunning the CI. |
|
/rebase |
6d448f4 to
5630faa
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
This PR updates the PageDetail property in FlyoutPage.cs.
The change simplifies the navigation logic by:
With this update,
SendNavigatingFromis invoked directly on the currentvalue.Issues Fixed
Fixes #30985
Tested the existing UI and DeviceTest in the following platforms