Skip to content

Conversation

@TheCodeTraveler
Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler commented Jun 22, 2025

Description of Change

This PR grants developers the capability to set global default values for every popup in their app using DefaultPopupOptionsSettings and DefaultPopupSettings in.UseMauiCommunityToolkit(Options):

.UseMauiCommunityToolkit(static options =>
{
  options.SetPopupDefaults(new DefaultPopupSettings
  {
	  CanBeDismissedByTappingOutsideOfPopup = true,
	  BackgroundColor = Colors.Orange,
	  HorizontalOptions = LayoutOptions.End,
	  VerticalOptions = LayoutOptions.Start,
	  Margin = 72,
	  Padding = 4
  });
  options.SetPopupOptionsDefaults(new DefaultPopupOptionsSettings
  {
	  CanBeDismissedByTappingOutsideOfPopup = true,
	  OnTappingOutsideOfPopup = () => hasOnTappingOutsideOfPopupExecuted = true,
	  PageOverlayColor = Colors.Orange,
	  Shadow = null,
	  Shape = null
  });
})

PR Checklist

Additional information

This PR also fixes a bug where View.Padding was improperly being passed through to PopupBorder.Padding

@TheCodeTraveler TheCodeTraveler added the pending documentation This feature requires documentation label Jun 22, 2025
@TheCodeTraveler TheCodeTraveler marked this pull request as ready for review June 22, 2025 21:57
Copilot AI review requested due to automatic review settings June 22, 2025 21:57
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 introduces global default settings for popups and popup options, allowing developers to configure defaults via UseMauiCommunityToolkit(Options), and refactors existing default classes. It also fixes how popup padding is applied and updates related bindings and tests.

  • Introduce DefaultPopupSettings and DefaultPopupOptionsSettings records and remove old static defaults.
  • Update PopupPage, Popup, converters, and Options to use the new default records and accept nullable options.
  • Refresh unit tests and sample to align with the new default-setting APIs.

Reviewed Changes

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

Show a summary per file
File Description
src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs Accept nullable options, bind new defaults, update converters
src/CommunityToolkit.Maui/Views/Popup/PopupOptions.shared.cs Change bindable property defaults to use Options.DefaultPopupOptionsSettings
src/CommunityToolkit.Maui/Views/Popup/Popup.shared.cs Initialize Popup defaults from Options.DefaultPopupSettings
src/CommunityToolkit.Maui/Primitives/Defaults/PopupOptionsDefaults.shared.cs Remove obsolete static defaults class
src/CommunityToolkit.Maui/Primitives/Defaults/PopupDefaults.shared.cs Remove obsolete static defaults class
src/CommunityToolkit.Maui/Primitives/DefaultPopupSettings.cs Add DefaultPopupSettings record
src/CommunityToolkit.Maui/Primitives/DefaultPopupOptionsSettings.cs Add DefaultPopupOptionsSettings record
src/CommunityToolkit.Maui/Options.cs Add storage and setters for default popup settings and options
src/CommunityToolkit.Maui/Extensions/PopupExtensions.shared.cs Drop fallback to PopupOptions.Empty, rely on nullable handling
samples/CommunityToolkit.Maui.Sample/MauiProgram.cs Invoke new SetPopupDefaults and SetPopupOptionsDefaults
Comments suppressed due to low confidence (5)

src/CommunityToolkit.Maui/Options.cs:17

  • The #error directive will block compilation for .NET 10 and later; remove the conditional branch and public constructor to prevent build failures and streamline the Options API.
#error Remove .NET 9 code now that we're upgrading to .NET 10; Options should not have a public constructor, only public methods. Having a public constructor allows developers to override the values set when using AppBuilderExtensions    

src/CommunityToolkit.Maui/Primitives/DefaultPopupSettings.cs:32

  • [nitpick] The nested class PopupDefaults inside DefaultPopupSettings conflicts in naming with the removed global PopupDefaults; consider renaming it to clarify its scope (e.g., DefaultValues).
	static class PopupDefaults

src/CommunityToolkit.Maui/Primitives/DefaultPopupOptionsSettings.cs:29

  • [nitpick] The nested class PopupOptionsDefaults inside DefaultPopupOptionsSettings shares a name with the old global defaults; renaming it (e.g., to DefaultValues) would reduce confusion.
	static class PopupOptionsDefaults

src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs:138

  • This binding no longer uses the VerticalOptionsConverter, so default fallback values won't be applied when view.VerticalOptions is Fill; add the converter to apply Options.DefaultPopupSettings.VerticalOptions.
		popup.SetBinding(Popup.VerticalOptionsProperty, static (View view) => view.VerticalOptions, source: view, mode: BindingMode.OneWay);

src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs:139

  • This binding no longer uses the HorizontalOptionsConverter, so default fallback values won't be applied when view.HorizontalOptions is Fill; add the converter to apply Options.DefaultPopupSettings.HorizontalOptions.
		popup.SetBinding(Popup.HorizontalOptionsProperty, static (View view) => view.HorizontalOptions, source: view, mode: BindingMode.OneWay);

@bijington
Copy link
Contributor

I know I initiated this thought process but I wanted to play devils advocate a little before we go further. I do like the idea of allowing developers to define defaults but I want to float some thoughts by you all to see what you think:

  • We should allow developers to control values through a Style which I believe is currently broken ([BUG] Padding and Margin are not applied to the Popup class of Popup2 by Style attribute #2747). I am not suggesting we fix it in this PR but I don't want this PR to make it any more difficult to fix
  • Perhaps a way to reduce the complexity of multiple possible values for things like CanDismiss... is to accept PopupOptions when passing in a View to the ShowPopup methods and not when passing in a Popup? I know this breaks some symmetry but with Popup but that might not be a bad thing. - I originally considered this but have struck it through because I think it causes more issues than it solves.
  • I would like us to make it possible for properties like BackgroundColor and PageOverlayColor to be set using an AppThemeBinding - yes it would be possible via a Style but should it be possible as a default too?

Copy link
Contributor

@bijington bijington left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @TheCodeTraveler good work on this! I have added 2 comments - one to help me understand what needs to be documented and another mostly just explaining why I took so long to review. Feel free to resolve the comments and merge.

@TheCodeTraveler TheCodeTraveler removed the pending documentation This feature requires documentation label Jul 3, 2025
@TheCodeTraveler TheCodeTraveler disabled auto-merge July 3, 2025 20:37
@TheCodeTraveler TheCodeTraveler merged commit a43bcef into main Jul 3, 2025
11 checks passed
@TheCodeTraveler TheCodeTraveler deleted the Add-Popup-Defaults-to-AppBuilderExtensions branch July 3, 2025 20:38
@github-actions github-actions bot locked and limited conversation to collaborators Jul 5, 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