Skip to content

Conversation

@TheCodeTraveler
Copy link
Collaborator

Description of Change

This PR updates the return type for Task PopupExtensions.ClosePopupAsync() -> Task<IPopupResult> PopupExtensions.ClosePopupAsync().

This PR also adds Task<IPopupResult<T>> PopupExtensions.ClosePopupAsync<T>().

PR Checklist

  • Has a linked Issue, and the Issue has been approved(bug) or Championed (feature/proposal)
  • Has tests (if omitted, state reason in description)
  • Has samples (if omitted, state reason in description)
  • Rebased on top of main at time of PR
  • Changes adhere to coding standard
  • Documentation created or updated: https://github.com/MicrosoftDocs/CommunityToolkit/pulls

Additional information

This PR also consolidates all of the Popup Open/Close logic to PopupExtensions.shared.cs; e.g. PopupService.CloseAsync() now calls PopupExtensions.ClosePopupAsync().

Copy link
Contributor

Copilot AI left a 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 changes the ClosePopupAsync API to return an IPopupResult (and a generic IPopupResult<T>) instead of void, adds generic overloads, and centralizes popup close logic into PopupExtensions.shared.cs.

  • Changed PopupService.ClosePopupAsync methods to forward to new extension methods
  • Added ClosePopupAsync and ClosePopupAsync<TResult> in PopupExtensions.shared.cs
  • Removed internal GetCurrentPopupPage and unified modal‐stack lookup

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

File Description
src/CommunityToolkit.Maui/Services/PopupService.shared.cs Delegated existing close logic to navigation extension methods
src/CommunityToolkit.Maui/Extensions/PopupExtensions.shared.cs Added new ClosePopupAsync overloads returning IPopupResult types
src/CommunityToolkit.Maui.UnitTests/Extensions/PopupExtensionsTests.cs Added tests for the new close‐with‐result APIs
Comments suppressed due to low confidence (1)

src/CommunityToolkit.Maui.UnitTests/Extensions/PopupExtensionsTests.cs:1280

  • Consider adding a test for the case where no popup is present (e.g. verify that calling ClosePopupAsync throws PopupNotFoundException).
[Fact(Timeout = (int)TestDuration.Short)]

pictos
pictos previously approved these changes May 27, 2025
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@TheCodeTraveler TheCodeTraveler requested a review from Copilot May 27, 2025 23:53
…Tests.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

Copilot AI left a 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 changes ClosePopupAsync to return IPopupResult (and a generic IPopupResult<T>), delegates the popup-close logic in PopupService to these extensions, and centralizes popup retrieval in GetMostRecentPopupPage.

  • Return types for all ClosePopupAsync methods updated to include IPopupResult.
  • Added generic ClosePopupAsync<T> overloads.
  • Consolidated popup lookup into GetMostRecentPopupPage and updated tests accordingly.

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

File Description
src/CommunityToolkit.Maui/Services/PopupService.shared.cs Simplified ClosePopupAsync methods to delegate to the updated extensions.
src/CommunityToolkit.Maui/Extensions/PopupExtensions.shared.cs Changed methods to return IPopupResult/IPopupResult<T>, refactored popup-close logic.
src/CommunityToolkit.Maui.UnitTests/Extensions/PopupExtensionsTests.cs Added tests for the new return values on page and navigation-based ClosePopupAsync.
Comments suppressed due to low confidence (1)

src/CommunityToolkit.Maui/Extensions/PopupExtensions.shared.cs:270

  • There’s no unit test for the scenario when no popup is present and GetMostRecentPopupPage should throw (e.g. PopupNotFoundException or PopupBlockedException). Consider adding a test for that error path.
static PopupPage GetMostRecentPopupPage(in INavigation navigation)

@TheCodeTraveler
Copy link
Collaborator Author

Thanks Pedro! 🙌

@TheCodeTraveler TheCodeTraveler merged commit 6184105 into main May 28, 2025
8 of 10 checks passed
@TheCodeTraveler TheCodeTraveler deleted the Return-`IPopupResult`-from-`PopupExtensions.ClosePopupAsync()` branch May 28, 2025 00:28
@github-actions github-actions bot locked and limited conversation to collaborators May 29, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants