-
Notifications
You must be signed in to change notification settings - Fork 442
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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. |
In fact I think this currently fails for |
There was a problem hiding this 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?
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 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? |
…kit/Maui into feature/sl-fix-popup-style
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
|
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()
Maui/src/CommunityToolkit.Maui/Views/Popup/PopupPage.shared.cs
Lines 186 to 220 in 28e030c
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); | |
} |
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
approved
(bug) orChampioned
(feature/proposal)main
at time of PRAdditional information