-
Notifications
You must be signed in to change notification settings - Fork 473
Refactor Layouts to use [BindableProperty] partial properties #3016
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
Refactor Layouts to use [BindableProperty] partial properties #3016
Conversation
TheCodeTraveler
left a comment
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 James!
Could you also please create a [ClassName]Defaults.shared.cs class for each of these Views in CommunityToolkit.Maui.Core/Defaults/ and also add a Unit Test for each called EnsureDefaults that verifies the default value for each property in the class equals the expected default value?
Here's an example that I added for FadeAnimation in the recent PR:
Maui/src/CommunityToolkit.Maui.Core/Primitives/Defaults/FadeAnimationDefaults.shared.cs
Lines 3 to 7 in f5b68d3
| static class FadeAnimationDefaults | |
| { | |
| public const uint Length = 300u; | |
| public const double Opacity = 0.3; | |
| } |
| [Fact] | |
| public void EnsureDefaults() | |
| { | |
| // Arrange | |
| var animation = new FadeAnimation(); | |
| // Act // Assert | |
| Assert.Equal(BaseAnimationDefaults.Easing, animation.Easing); | |
| Assert.Equal(FadeAnimationDefaults.Length, animation.Length); | |
| Assert.Equal(FadeAnimationDefaults.Opacity, animation.Opacity); | |
| } |
We should've created [ClassName]Defaults.shared.cs and implemented these Unit Tests when originally creating every control in CommunityToolkit.Maui. But now that we're going back and editing every control to implement [BindableProperty], I've found that this is a great time to clean up our codebase and implement these!
Sounds like a plan. I will make the requested changes. |
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 refactors two layout classes (UniformItemsLayout and DockLayout) to use modern [BindableProperty] partial properties instead of traditional BindableProperty definitions. The refactoring introduces default value constants in new static classes and adds unit tests to verify default values are correctly set.
Key changes:
- Migrates bindable properties to use source-generated partial properties with
[BindableProperty]attributes - Extracts default values into centralized
UniformItemLayoutDefaultsandDockLayoutDefaultsclasses - Adds unit tests to validate default property values
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| UniformItemsLayout.shared.cs | Refactored MaxRows and MaxColumns properties to use [BindableProperty] with partial properties; moved validation to property changed callbacks |
| DockLayout.shared.cs | Refactored ShouldExpandLastChild, HorizontalSpacing, and VerticalSpacing properties to use [BindableProperty] with partial properties |
| UniformItemLayoutDefaults.shared.cs | New static class defining default constants for UniformItemsLayout properties |
| DockLayoutDefaults.shared.cs | New static class defining default constants for DockLayout properties |
| UniformItemsLayoutTests.cs | Added test to verify default property values; added necessary namespace import |
| DockLayoutTests.cs | Added test to verify default property values |
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
TheCodeTraveler
left a comment
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 James! Let's go ahead and merge this.
I've included the Attached Bindable Property updates for DockLayout in my [AttachedBindableProperty] PR: #3024
8051299
into
CommunityToolkit:main
Description of Change
This adds support for
[BindableProperties]toLayoutsin CommunityToolkitMaui.This PR is blocked until support is added for
BindableProperties.AddAttachedas it is required to complete this PR. There are several classesStateContainer,StateContainerControllerandStateViewthat require addition updates to our internal source generator to complete this.PR Checklist
approved(bug) orChampioned(feature/proposal)mainat time of PRAdditional information
Blocked until support for Attached bindable properties is added.