-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Correctly propagate BindingContext to Border StrokeShape #13793
Conversation
|
||
if (strokeShape is VisualElement visualElement) | ||
{ | ||
SetInheritedBindingContext(visualElement, BindingContext); |
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.
We were not propagating the BindingContext to the Shape used in the StrokeShape
property or detecting changes to invalidate the Shape. These are the main 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.
Do we need Parent AND SetInheritedBindingContext? Does not setting Parent automatically set the binding context? I am not sure, so just checking... Does the order matter?
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
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.
Nice! Just got a question on the propagation and the use of the new proxy types.
|
||
if (strokeShape is VisualElement visualElement) | ||
{ | ||
SetInheritedBindingContext(visualElement, BindingContext); |
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.
Do we need Parent AND SetInheritedBindingContext? Does not setting Parent automatically set the binding context? I am not sure, so just checking... Does the order matter?
Thank you for your pull request. We are auto-formatting your source code to follow our code guidelines. |
Added sample to validate #13988 |
propertyChanging: (bindable, oldvalue, newvalue) => | ||
{ | ||
if (newvalue is not null) | ||
(bindable as Border)?.StopNotifyingStrokeShapeChanges(); |
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.
Should this always fire? If I set a value and then null, this will not unsub.
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.
If this is wrong, can we add a test for this?
propertyChanging: (bindable, oldvalue, newvalue) => | ||
{ | ||
if (newvalue is not null) | ||
(bindable as Border)?.StopNotifyingStrokeChanges(); |
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.
Same here, nothing ever unsubs.
} |
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.
Can we revert this whitespace change?
Description of Change
This PR apply changes to correctly propagate the Border BindingContext to the StrokeShape.
Added a new sample in the Gallery where the Border StrokeShape bind the CornerRadius to the Border Width.
And unit test.
Issues Fixed
Fixes #13699
Fixes #13988