Popup V2 #1581

bijington · 2023-11-29T21:46:51Z

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

Fixes [Proposal] Popup V2 #2434

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

Migration Guide

Popup XAML/CS

Remove CanBeDismissedByTappingOutsideOfPopup and pass parameter using PopupOptions
Color is replaced with Background Color
Size is replaced with WidthRequest and HeightRequest

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

All popup configurations can be configured using PopupOptions and passed as a parameter
You need to pass INavigation to make sure the popup is displayed on the correct window.

ne0rrmatrix · 2023-12-01T02:39:34Z

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.

src/CommunityToolkit.Maui/PopupService.cs

bijington · 2024-02-11T14:20:37Z

There are currently a couple of breaking changes to the IPopupService that I think are still good but we should consider documenting them to aid developers upgrading:

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 viewModel instance to the Show methods to remove the confusion we have seen around developers expecting the BindingContext to automatically be set. I am happy to move this into a separate PR if we think it if worthwhile?

TheCodeTraveler · 2024-03-26T00:34:49Z

@bijington FYI - we now have a few merge conflicts on this PR after merging a bunch of Popup PRs today

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

pictos

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.

src/CommunityToolkit.Maui/Views/Popup/PopupContainer.cs

src/CommunityToolkit.Maui/Views/Popup/PopupExtensions.cs

src/CommunityToolkit.Maui/Views/Popup/PopupService.cs

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 = "";

…able properties The aim is to allow us to hide the Popup away from developers, or at least simply the usage of showing a popup to not needing to create a Popup implementation

Completed

…p-service

pictos

I did try the Cannot Be Dismissed By Tapping OUtside POpup sample and when you tap more than once on Close button this exception will raise.

you can try on any sample that allows CanBeDismissedByTappingOutsideOfPopup

So we probably want to change it to (inside PopupContainer and PopupContainer<T>)

		taskCompletionSource.TrySetResult(result);

But after that, another exception occurs

so we probably will need a defensive code.

pictos · 2025-02-11T20:03:52Z

src/CommunityToolkit.Maui/Views/Popup/PopupLifecycleController.cs

 {
-	readonly List<WeakReference<Popup>> currentPopups = [];
+	readonly List<WeakReference<PopupContainer>> currentPopups = [];


There're scenarios in .net maui that Window and Application will recreated (if user doesn't cache them) which can cause this List to increase with invalid WeakReference, would be good to have a logic that will lookup into the list and remove invalid WeakReferences

pictos · 2025-02-11T20:07:35Z

src/CommunityToolkit.Maui/Views/Popup/PopupExtensions.cs

+				Command = new Command(async () =>
+				{
+					options.OnTappingOutsideOfPopup?.Invoke();
+					await popupContainer.Close(new PopupResult(true));


can we cache this PopupResult? And also the PopupResult false

pictos · 2025-02-11T20:11:19Z

src/CommunityToolkit.Maui/Views/Popup/PopupContainer.cs

+	readonly TaskCompletionSource<PopupResult>? taskCompletionSource;
+
+	public PopupContainer(Popup content, TaskCompletionSource<PopupResult>? taskCompletionSource)
+	{


shouldn't we add argument validation for content ?

what should we validate?

If content isn't null

pictos · 2025-02-11T20:12:13Z

src/CommunityToolkit.Maui/Views/Popup/PopupContainer.cs

+{
+	readonly TaskCompletionSource<PopupResult<T>> taskCompletionSource;
+
+	public PopupContainer(Popup<T> content, TaskCompletionSource<PopupResult<T>> taskCompletionSource) :base (content, null)


shouldn't we add argument validation for taskCompletionSource? (content will be validated on base ctor)

ne0rrmatrix · 2025-02-11T20:45:13Z

When clicking on Updating Popup and selecting More after it has run and returning it will go through original again a second time. Is this intended?

On popup position page it only works once on the first popup you select. The button is not reactive and does not dismiss by clicking ok. If you try and select a second popup nothing happens. Leaving the page returning is the only way to test other items.

On popup anchor page the button is not clickable and only works once. After failing to click you can tap outside the popup to close it. But the Show Popup button is greyed out and cannot be clicked.

ne0rrmatrix · 2025-02-11T20:51:04Z

samples/CommunityToolkit.Maui.Sample/ViewModels/Views/Popup/PopupContentViewModel.cs

+public sealed partial class PopupContentViewModel : BaseViewModel
+{
+    [ObservableProperty]
+	string message = "";


Can we replace this with:

[ObservableProperty] public partial string Message { get; set; } = string.empty;

bijington · 2025-02-11T21:07:46Z

When clicking on Updating Popup and selecting More after it has run and returning it will go through original again a second time. Is this intended?

The idea behind this was to allow for stacking of Popups and to test things around that

…p-service

bijington added the needs discussion Discuss it on the next Monthly standup label Dec 5, 2023

TheCodeTraveler removed the needs discussion Discuss it on the next Monthly standup label Jan 4, 2024

VladislavAntonyuk reviewed Feb 8, 2024

View reviewed changes

src/CommunityToolkit.Maui/PopupService.cs Outdated Show resolved Hide resolved

bijington marked this pull request as ready for review February 11, 2024 14:26

dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Apr 27, 2024

bijington removed help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days labels Jun 5, 2024

bijington requested a review from a team June 9, 2024 20:43

bijington changed the title ~~Initial musings on showing popups without having to subclass Popup~~ Provide the ability to show popups without having to subclass Popup Jun 20, 2024

dotnet-policy-service bot added stale The author has not responded in over 30 days help wanted This proposal has been approved and is ready to be implemented labels Jul 22, 2024

bijington requested review from pictos and VladislavAntonyuk October 15, 2024 18:31

VladislavAntonyuk previously approved these changes Oct 30, 2024

View reviewed changes

VladislavAntonyuk requested a review from Copilot December 10, 2024 23:13

Copilot AI reviewed Dec 10, 2024

View reviewed changes

VladislavAntonyuk dismissed their stale review via 49da03d January 14, 2025 12:19

VladislavAntonyuk force-pushed the feature/sl/remove-popup-constraint-from-popup-service branch from c1488d0 to ebc39f2 Compare January 15, 2025 00:11

VladislavAntonyuk self-assigned this Jan 15, 2025

VladislavAntonyuk force-pushed the feature/sl/remove-popup-constraint-from-popup-service branch from aeb9020 to 94bee57 Compare January 15, 2025 18:52

pictos requested changes Jan 15, 2025

View reviewed changes

VladislavAntonyuk requested a review from Copilot January 15, 2025 20:31

Copilot AI reviewed Jan 15, 2025

View reviewed changes

VladislavAntonyuk added pending documentation This feature requires documentation and removed help wanted This proposal has been approved and is ready to be implemented stale The author has not responded in over 30 days labels Jan 15, 2025

bijington and others added 18 commits February 6, 2025 21:38

Pass on the BindingContext of a view up to the popup

f7847ec

Expand IPopupService API to enable the ability to supply the configur…

e0ac620

…able properties The aim is to allow us to hide the Popup away from developers, or at least simply the usage of showing a popup to not needing to create a Popup implementation

Name the cancellation token usage

b153f60

Include DynamicallyAccessedMembers

4fcbf83

Popup V2

45a35b3

Update samples

2716adc

Fix tests

3fce702

register all popups

fbba90d

fix tizen build

ccf952d

await tasks

6afff1f

fix xaml

b097df1

Fix tests

45ab59d

Fix popup Result

8b535bf

Fix tests, fix samples, rework popup service

10cb099

fix sample

133b398

Fix back button

2de506b

Fix tests

8ed786c

Add Navigation parameter

df0b6a3

VladislavAntonyuk force-pushed the feature/sl/remove-popup-constraint-from-popup-service branch from 80f3920 to df0b6a3 Compare February 6, 2025 19:46

Fix after rebase

d5cb5a1

VladislavAntonyuk and others added 4 commits February 7, 2025 00:23

Remove old page

e9cf804

Merge branch 'main' into feature/sl/remove-popup-constraint-from-popu…

f1d99c0

…p-service

Add docs

6818cb8

Merge branch 'main' into feature/sl/remove-popup-constraint-from-popu…

df907ea

…p-service

pictos requested changes Feb 11, 2025

View reviewed changes

ne0rrmatrix reviewed Feb 11, 2025

View reviewed changes

Merge branch 'main' into feature/sl/remove-popup-constraint-from-popu…

0616afb

…p-service

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Popup V2 #1581

Popup V2 #1581

bijington commented Nov 29, 2023 •

edited by VladislavAntonyuk

Loading

ne0rrmatrix commented Dec 1, 2023 •

edited

Loading

bijington commented Feb 11, 2024

TheCodeTraveler commented Mar 26, 2024

pictos left a comment

pictos left a comment •

edited

Loading

pictos Feb 11, 2025

pictos Feb 11, 2025 •

edited

Loading

pictos Feb 11, 2025

VladislavAntonyuk Feb 12, 2025

pictos Feb 12, 2025

pictos Feb 11, 2025

ne0rrmatrix commented Feb 11, 2025

ne0rrmatrix Feb 11, 2025

bijington commented Feb 11, 2025

Popup V2 #1581

Are you sure you want to change the base?

Popup V2 #1581

Conversation

bijington commented Nov 29, 2023 • edited by VladislavAntonyuk Loading

Description of Change

Linked Issues

PR Checklist

Migration Guide

Popup XAML/CS

Popup Service

ne0rrmatrix commented Dec 1, 2023 • edited Loading

bijington commented Feb 11, 2024

TheCodeTraveler commented Mar 26, 2024

Choose a reason for hiding this comment

pictos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pictos left a comment • edited Loading

Choose a reason for hiding this comment

pictos Feb 11, 2025

Choose a reason for hiding this comment

pictos Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

pictos Feb 11, 2025

Choose a reason for hiding this comment

VladislavAntonyuk Feb 12, 2025

Choose a reason for hiding this comment

pictos Feb 12, 2025

Choose a reason for hiding this comment

pictos Feb 11, 2025

Choose a reason for hiding this comment

ne0rrmatrix commented Feb 11, 2025

ne0rrmatrix Feb 11, 2025

Choose a reason for hiding this comment

bijington commented Feb 11, 2025

bijington commented Nov 29, 2023 •

edited by VladislavAntonyuk

Loading

ne0rrmatrix commented Dec 1, 2023 •

edited

Loading

pictos left a comment •

edited

Loading

pictos Feb 11, 2025 •

edited

Loading