Popup V2 #1581
Popup V2 #1581
Conversation
|
I have ended up rewriting at least 3 or 4 samples submitted as bugs using standard DI pattern because the code would crash in IOS 17.x. I wanted to verify the issues that were being reported. If I see a bunch of different reports I put extra effort in. Simplifying implementation would be great. I am all for that. |
TheCodeTraveler
removed
the
needs discussion
|
There are currently a couple of breaking changes to the await popupService.ShowPopupAsync<MockPageViewModel>(CancellationToken.None);becomes await popupService.ShowPopupAsync<MockPageViewModel>(token: CancellationToken.None);await Assert.ThrowsAsync<TaskCanceledException>(() => popupService.ShowPopupAsync<MockPageViewModel>(viewModel => viewModel.HasLoaded = true, cts.Token));becomes await Assert.ThrowsAsync<TaskCanceledException>(() => popupService.ShowPopupAsync<MockPageViewModel>(viewModel => viewModel.HasLoaded = true, token: cts.Token));It also involves removing the ability to supply a |
|
@bijington FYI - we now have a few merge conflicts on this PR after merging a bunch of Popup PRs today |
dotnet-policy-service
bot
added
stale
bijington
removed
help wanted
bijington
changed the title
dotnet-policy-service
bot
added
stale
There was a problem hiding this comment.
Copilot reviewed 5 out of 11 changed files in this pull request and generated no suggestions.
Files not reviewed (6)
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/MultiplePopupPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Views/Popups/PopupContentView.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/MauiProgram.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/MultiplePopupViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupContentViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/Views/Popups/PopupContentView.xaml.cs: Evaluated as low risk
VladislavAntonyuk
force-pushed
the
feature/sl/remove-popup-constraint-from-popup-service
branch
from
c1488d0 to
ebc39f2
Compare
VladislavAntonyuk
force-pushed
the
feature/sl/remove-popup-constraint-from-popup-service
branch
from
aeb9020 to
94bee57
Compare
There was a problem hiding this comment.
Good job! I didn't run locally yet, just reviewed the code.
I saw that you removed the old popups reference, and I think we should mark them as obsolete and let them live for a while before removing everything. At least is what I remember from the last standup.
There was a problem hiding this comment.
Copilot reviewed 81 out of 96 changed files in this pull request and generated no comments.
Files not reviewed (15)
- samples/CommunityToolkit.Maui.Sample/App.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/MultiplePopupPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Resources/Styles/Styles.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Models/PopupSize.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/AppShell.xaml.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupSizingIssuesViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/StylePopupViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupPositionViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/CustomSizeAndPositionPopupViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/UpdatingPopupViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupLayoutAlignmentPage.xaml.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/ViewsGalleryViewModel.cs: Evaluated as low risk
- samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupAnchorViewModel.cs: Evaluated as low risk
Comments suppressed due to low confidence (1)
samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupContentViewModel.cs:8
- The property name 'message' should be renamed to 'Message' to follow PascalCase convention.
string message = "";
ne0rrmatrix
dismissed
their stale review
Approved before it was ready. My bad. I was excited :(
|
I am excited to see this and I don't have any suggestions at this time. I hope to see this go live soon! How are we on documentation? Has it been started? Or are we waiting on @bijington to review it? |
|
Thanks for the reviews! We are now down to just 3 items that we need to resolve before we can merge Popup v2:
I'll follow up with the maintainer team to vote on these last remaining questions 🙌 |
TheCodeTraveler
added
the
needs discussion
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and modernizes popup handling in the .NET MAUI Toolkit samples by replacing legacy direct popup calls with a centralized IPopupService and by removing deprecated popup dependencies. Key changes include:
- Replacing usages of PopupSizeConstants with IPopupService injection and flag-based popup display control.
- Adding new sample pages (e.g. PopupsPage) that demonstrate various popup behaviors.
- Removing obsolete popup pages and adjusting popup registrations and view model mappings accordingly.
Reviewed Changes
Copilot reviewed 112 out of 120 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml.cs | Updated to inject IPopupService and display a popup once upon appearing using a flag. |
| samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupsPage.xaml.cs | Introduces multiple async handlers to demonstrate various popup scenarios. |
| samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupOnDisappearingPage.cs | Displays a popup on page disappearance using ShowPopupAsync. |
| samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupLayoutAlignmentPage.xaml.cs | Refactored to use local layout option variables and streamlined popup display. |
| samples/CommunityToolkit.Maui.Sample/Pages/Views/MediaElement/MediaElementPage.xaml.cs | Modified the popup display in media element scenarios to use ShowPopupAsync. |
| samples/CommunityToolkit.Maui.Sample/MauiProgram.cs | Removed deprecated PopupSizeConstants registration and updated popup registrations. |
| samples/CommunityToolkit.Maui.Sample/AppShell.xaml.cs | Adjusted view model mappings to reflect new sample page structure. |
Files not reviewed (8)
- Directory.Build.props: Language not supported
- samples/CommunityToolkit.Maui.Sample/App.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/CustomSizeAndPositionPopupPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupAnchorPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupSizingIssuesPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupsPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/ShowPopupInOnAppearingPage.xaml: Language not supported
- samples/CommunityToolkit.Maui.Sample/Resources/Styles/Styles.xaml: Language not supported
samples/CommunityToolkit.Maui.Sample/Pages/Views/Popup/PopupOnDisappearingPage.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I observe that the PR removes the outdated Popup implementation, and the main file lacks the Obsolete attribute. Consequently, this constitutes a significant breaking change that will not provide other developers with sufficient time to migrate to the new Popup implementation. Furthermore, this action isn't the pattern followed by the official .NET libraries. Therefore, I strongly recommend reverting the deleted files and adding the Obsolete attribute.
|
|
||
| if (popupPageToClose is null) | ||
| { | ||
| throw new InvalidOperationException($"Unable to close popup: could not locate {nameof(PopupPage)}. If using a custom implementation of {nameof(Popup)}, override the {nameof(Close)} method"); |
There was a problem hiding this comment.
not sure if should throw for here, I can't see good reason to do an exception if the popup can't be closed and not given a way for devs to leave here. In other words, let's say that we have 3 popups and for some reason I can't close the last one, then my app will crash (if the exception isn't handled) or will be blocked on those popups forever.
Maybe we can throw and add a method CloseAllPopups that will close all popups opened popups?
There was a problem hiding this comment.
The Exception is valid.
When the developer calls Close(), we have two options:
- Close the current Popup
- Throw an exception telling them why we were unable to close the Popup
There was a problem hiding this comment.
Yeap, but in a production code, the app will be blocked forever on the popup. So I would say that we should provide a panic mode, a method that will close all popups.
I believe it's easy to implement a method like that and expose them for all devs, that way they can decide how to handle this situation better
There was a problem hiding this comment.
The scenarios where we throw an InvalidOperationException are very rare and I doubt many (if any) developers will run into either.
the app will be blocked forever on the popup
The good news is that both scenarios have work-arounds ensuring that we don't block developers:
- Developers have implemented their own implementation of
Popupthat bypassesPopupPage- The solution here, as the Exception message explains, is to override the
Popup.Close()method
- The solution here, as the Exception message explains, is to override the
- A developer has pushed a Modal Page on top of the Popup
- The solution here, as the Exception message explains, is to ensure all modal pages are dismissed before calling
.Close()(example solution below)
- The solution here, as the Exception message explains, is to ensure all modal pages are dismissed before calling
while(Navigation.ModalStack.Count > 1)
{
await Navigation.PopModalAsync();
}
await popup.Close();There was a problem hiding this comment.
I'll be sure to work with Shaun to add this to the docs 👍
Co-authored-by: Pedro Jesus <pedrojesus.cefet@gmail.com>
Description of Change
This is primarily aimed at playing around but also with the aim of exposing the implementation to see what everything thinks.
The popupcontainer is an internal ContentPage that is used to display content. there are 2 ways user can create popup. It can be a simple view or for more complex tasks user has to inherit from Popup. What are the complex tasks? In case you want to Close popup from the control or you want to get the result from the popup or you want to subscribe on OnOpened/OnClosed.
For MVVM users there is a PopupService. Pay attention all popups must be registered. Also you can close popup from PopupService only if you opened it using PopupService. DO NOT combine extension method and PopupService.
Now all configurations live in PopupOptions. You can configure BackgroundColor (Dim), popup size and popup layout position. Dissmiss by outside tap also exist in PopupOptions.
The BindingContext is set automatically to PopupContainer, Popup and View.
As the new Popup uses only MAUI Controls, we expect you get all benefits of HotReload. All Controls including Popup in Popup and MediaElement should work without issues.
Features currently removed: AnchorView.
Linked Issues
FlyoutBehaviour.Lockedon macOS #2566OnLoadedPopup Workaround #1490FlyoutBehaviour.Lockedon macOS #2566PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRMigration Guide
Popup XAML/CS
You can now use any View as Popup Content without inheriting it from Popup. But if you need to return a result from the popup, you have to inherit from Popup.
Popup Service