Bindable property for Popup #2991
Conversation
Replaced manual BindableProperty implementations with the BindableProperty attribute in the `Popup` and `PopupOptions` classes. This simplifies property definitions, reduces boilerplate, and improves code readability. Updated properties such as `Margin`, `Padding`, `HorizontalOptions`, and `VerticalOptions` in `Popup`, and `CanBeDismissedByTappingOutsideOfPopup`, `OnTappingOutsideOfPopup`, `PageOverlayColor`, `Shape`, and `Shadow` in `PopupOptions` to use the attribute with default value creators. Removed redundant manual property implementations as they are now handled by the BindableProperty attribute.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the Popup and PopupOptions classes to use the [BindableProperty] attribute for auto-generating bindable properties instead of manually declaring them. While the intent to modernize the code is good, there are several critical bugs that prevent this code from working correctly.
Key Changes:
- Replaced manual
BindablePropertydeclarations with[BindableProperty]attributes in bothPopupandPopupOptionsclasses - Improved XML documentation for better clarity
- Removed redundant property wrappers and backing fields
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/CommunityToolkit.Maui/Views/Popup/Popup.shared.cs | Added using System.ComponentModel;, converted 5 properties to use [BindableProperty] attribute with default value creators, and removed manual property implementations |
| src/CommunityToolkit.Maui/Views/Popup/PopupOptions.shared.cs | Converted 5 properties to use [BindableProperty] attribute with default value creators and removed manual property implementations |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
| public partial Action? OnTappingOutsideOfPopup { get; set; } | ||
| static object? CreateOnTappingOutsideOfPopup(BindableObject? _) => Options.DefaultPopupOptionsSettings.OnTappingOutsideOfPopup; |
There was a problem hiding this comment.
Nullability mismatch: The property is declared as non-nullable Action, but the default value creator method returns object? (nullable). This creates an inconsistency where the property signature suggests it's always non-null, but the default value can be null.
Consider either:
- Making the property nullable:
public partial Action? OnTappingOutsideOfPopup { get; set; } - Providing a non-null default value if the property should never be null
Note: The interface IPopupOptions.OnTappingOutsideOfPopup is nullable (Action?), so option 1 is recommended for consistency.
| public static readonly BindableProperty PageOverlayColorProperty = BindableProperty.Create(nameof(PageOverlayColor), typeof(Color), typeof(PopupOptions), Options.DefaultPopupOptionsSettings.PageOverlayColor); | ||
| [BindableProperty(DefaultValueCreatorMethodName = nameof(CreatePageOverlayColor))] | ||
| public partial Color PageOverlayColor { get; set; } | ||
| static object CreatePageOverlayColor(BindableObject? _) => Options.DefaultPopupOptionsSettings.PageOverlayColor; |
There was a problem hiding this comment.
[nitpick] Nullability mismatch: The property is declared as non-nullable Color, but the default value creator method can potentially return a nullable value if Options.DefaultPopupOptionsSettings.PageOverlayColor is null. While Color is a struct and typically non-nullable, this could cause issues if the default settings are not properly initialized.
Verify that Options.DefaultPopupOptionsSettings.PageOverlayColor is guaranteed to be non-null, or update the default value creator to handle potential null cases explicitly.
| static object CreatePageOverlayColor(BindableObject? _) => Options.DefaultPopupOptionsSettings.PageOverlayColor; | |
| static object CreatePageOverlayColor(BindableObject? _) | |
| { | |
| return Options.DefaultPopupOptionsSettings.PageOverlayColor is not null | |
| ? Options.DefaultPopupOptionsSettings.PageOverlayColor | |
| : Colors.Transparent; | |
| } |
| @@ -1,3 +1,4 @@ | |||
| using System.ComponentModel; | |||
There was a problem hiding this comment.
[nitpick] The using System.ComponentModel; directive doesn't appear to be used in this file. The [BindableProperty] attribute is defined in the CommunityToolkit.Maui namespace (auto-generated by the source generator), not System.ComponentModel.
If this directive is not used elsewhere in the file, consider removing it to keep imports clean.
| using System.ComponentModel; |
Updated the `CanBeDismissedByTappingOutsideOfPopup` property to use a `DefaultValueCreatorMethodName` for dynamic default value resolution. Added the `CreateCanBeDismissedByTappingOutsideOfPopup` method to provide the default value at runtime, improving flexibility and reducing reliance on hardcoded defaults.
1 participant
Description of Change
This pull request refactors how bindable properties are defined and used in the
PopupandPopupOptionsclasses. Instead of manually declaring backingBindablePropertyfields and property wrappers, it leverages the[BindableProperty]attribute to auto-generate these properties, resulting in cleaner and more maintainable code. Additionally, the XML documentation for properties has been improved for clarity.Refactoring of bindable properties:
BindablePropertydeclarations and property wrappers inPopup(Popup.shared.cs) with auto-generated properties using the[BindableProperty]attribute forMargin,Padding,HorizontalOptions,VerticalOptions, andCanBeDismissedByTappingOutsideOfPopup.PopupOptions(PopupOptions.shared.cs) to use[BindableProperty]forCanBeDismissedByTappingOutsideOfPopup,OnTappingOutsideOfPopup,PageOverlayColor,Shape, andShadow, removing manual property logic.Documentation improvements:
PopupandPopupOptionsto be more descriptive and consistent. [1] [2]Code cleanup:
General:
using System.ComponentModel;directive to support the[BindableProperty]attribute.PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Not sure if using a static object is best approach but it works. I am hoping for input on whether to use that or if anyone has any better ideas?