Skip to content

Move default values into a style to support style overriding #2770

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bijington
Copy link
Contributor

Description of Change

I haven't fully tested this but from what I can see it certainly seems to solve the lack of Style support for the Popup properties. I suspect there may be a more efficient way to do this too, perhaps a single instance of the default style?

Linked Issues

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

@Copilot Copilot AI review requested due to automatic review settings June 25, 2025 21:02
Copy link
Contributor

@Copilot 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 moves the default property values for the Popup into a style so that they can be overridden, and adjusts the binding setups accordingly in both the Popup and PopupPage components.

  • Default Popup property assignments now use a style added to the Popup’s Resources rather than inline assignments.
  • Bindings in PopupPage.shared.cs have been updated to use statically imported property references.
  • The sample App.xaml has been updated to demonstrate style overriding for Popup properties.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs Updated binding calls to use statically imported properties instead of explicit Popup properties.
src/CommunityToolkit.Maui/Views/Popup/Popup.shared.cs Removed redundant BindableProperty declarations and moved default value assignments into a newly created style.
samples/CommunityToolkit.Maui.Sample/App.xaml Added setters for Popup styling to support style overriding.

@bijington
Copy link
Contributor Author

In fact I think this currently fails for ImplicitStylePopup

Copy link
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

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

Thanks Shaun! I hadn't considered Style when working on Popup v2 and boy was that a glaring mistake.

Is this PR ready, or does it need more work for implicit styles like you mentioned in your comment above?

@bijington
Copy link
Contributor Author

Thanks Shaun! I hadn't considered Style when working on Popup v2 and boy was that a glaring mistake.

Is this PR ready, or does it need more work for implicit styles like you mentioned in your comment above?

I can't decide if the behaviour is correct or not. Currently the ImplicitStylePopup will fill full screen because it doesn't define a value for HorizontalOptions or VerticalOptions. It does provide HeightRequest and WidthRequest which has no effect because of the first two that I mentioned.

I am not thinking that this behaviour is correct because this implicit style overrides our implicit style in the toolkit - there can only be one implicit style to use right?

@TheCodeTraveler
Copy link
Collaborator

TheCodeTraveler commented Jun 26, 2025

I can't decide if the behaviour is correct or not. Currently the ImplicitStylePopup will fill full screen because it doesn't define a value for HorizontalOptions or VerticalOptions. It does provide HeightRequest and WidthRequest which has no effect because of the first two that I mentioned.

We don't want it to fill the screen by default; it should be centered by default.

I haven't had time to dig into the code - just a thought - this may be related to how we're propagating the HorizontalOptions and VerticalOptions via Bindings from the View that the user gives us onto the underlying Popup and underlying PopupBorder in PopupPage. CreatePopupFromView<T>() and PopupPage.PopupPageLayout():

PopupPage. CreatePopupFromView<T>()

protected static T CreatePopupFromView<T>(in View view) where T : Popup, new()
{
ArgumentNullException.ThrowIfNull(view);
var popup = new T
{
BackgroundColor = view.BackgroundColor ??= PopupDefaults.BackgroundColor,
Content = view
};
popup.SetBinding(BackgroundProperty, static (View view) => view.Background, source: view, mode: BindingMode.OneWay);
popup.SetBinding(BindingContextProperty, static (View view) => view.BindingContext, source: view, mode: BindingMode.OneWay);
popup.SetBinding(BackgroundColorProperty, static (View view) => view.BackgroundColor, source: view, mode: BindingMode.OneWay);
popup.SetBinding(Popup.MarginProperty, static (View view) => view.Margin, source: view, mode: BindingMode.OneWay);
popup.SetBinding(Popup.VerticalOptionsProperty, static (View view) => view.VerticalOptions, source: view, mode: BindingMode.OneWay, converter: new VerticalOptionsConverter());
popup.SetBinding(Popup.HorizontalOptionsProperty, static (View view) => view.HorizontalOptions, source: view, mode: BindingMode.OneWay, converter: new HorizontalOptionsConverter());
if (view is IPaddingElement paddingElement)
{
popup.SetBinding(Popup.PaddingProperty, static (IPaddingElement paddingElement) => paddingElement.Padding, source: paddingElement, mode: BindingMode.OneWay, converter: new PaddingConverter());
}
return popup;

PopupPage.PopupPageLayout()

public PopupPageLayout(in Popup popupContent, in IPopupOptions options, in ICommand tapOutsideOfPopupCommand)
{
Background = BackgroundColor = null;
var tappableBackground = new BoxView
{
BackgroundColor = Colors.Transparent,
HorizontalOptions = LayoutOptions.Fill,
VerticalOptions = LayoutOptions.Fill
};
tappableBackground.GestureRecognizers.Add(new TapGestureRecognizer { Command = tapOutsideOfPopupCommand });
Children.Add(tappableBackground); // Add the Tappable Background to the PopupPageLayout Grid before adding the Border to ensure the Border is displayed on top
PopupBorder = new Border
{
BackgroundColor = popupContent.BackgroundColor ??= PopupDefaults.BackgroundColor,
Content = popupContent
};
// Bind `Popup` values through to Border using OneWay Bindings
PopupBorder.SetBinding(Border.MarginProperty, static (Popup popup) => popup.Margin, source: popupContent, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.PaddingProperty, static (Popup popup) => popup.Padding, source: popupContent, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.BackgroundProperty, static (Popup popup) => popup.Background, source: popupContent, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.BackgroundColorProperty, static (Popup popup) => popup.BackgroundColor, source: popupContent, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.VerticalOptionsProperty, static (Popup popup) => popup.VerticalOptions, source: popupContent, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.HorizontalOptionsProperty, static (Popup popup) => popup.HorizontalOptions, source: popupContent, mode: BindingMode.OneWay);
// Bind `PopupOptions` values through to Border using OneWay Bindings
PopupBorder.SetBinding(Border.ShadowProperty, static (IPopupOptions options) => options.Shadow, source: options, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.StrokeProperty, static (IPopupOptions options) => options.Shape, source: options, converter: new BorderStrokeConverter(), mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.StrokeShapeProperty, static (IPopupOptions options) => options.Shape, source: options, mode: BindingMode.OneWay);
PopupBorder.SetBinding(Border.StrokeThicknessProperty, static (IPopupOptions options) => options.Shape, source: options, converter: new BorderStrokeThicknessConverter(), mode: BindingMode.OneWay);
Children.Add(PopupBorder);
}

@TheCodeTraveler TheCodeTraveler added pending documentation This feature requires documentation pending unit tests Indicates that a PR requires unit tests before it can be merged labels Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pending documentation This feature requires documentation pending unit tests Indicates that a PR requires unit tests before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Padding and Margin are not applied to the Popup class of Popup2 by Style attribute
2 participants