-
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
Applying global styles to shadows #20203
base: main
Are you sure you want to change the base?
Conversation
Hey there @kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Hey @kubaflo Thanks for the suggested PR, could you finish the merge of this? |
@kubaflo Could you rebase to fix the conflict?, thnx! |
d80ea59
to
8058667
Compare
@jsuarezruiz rebased, and, thanks to it, only one change was needed to make it work :) |
@@ -19,6 +19,13 @@ public class Shadow : Element, IShadow | |||
|
|||
Paint IShadow.Paint => Brush; | |||
|
|||
internal MergedStyle _mergedStyle; | |||
|
|||
public Shadow() |
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.
this doesn't look like the right fix. The problem probably comes from the Shadow isn't properly parented.
(parenting code was probably removed to 'fix' a memory leak, but we have a PR avoiding that leak)
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.
@StephaneDelcroix I tried a solution with setting the parent visual element as a parent here:
maui/src/Controls/src/Core/VisualElement/VisualElement.cs
Lines 309 to 325 in 5d67531
void NotifyBackgroundChanges() | |
{ | |
var background = Background; | |
if (background is ImmutableBrush) | |
return; | |
if (background != null) | |
{ | |
SetInheritedBindingContext(background, BindingContext); | |
_backgroundChanged ??= (sender, e) => OnPropertyChanged(nameof(Background)); | |
_backgroundProxy ??= new(); | |
_backgroundProxy.Subscribe(background, _backgroundChanged); | |
OnParentResourcesChanged(this.GetMergedResources()); | |
((IElementDefinition)this).AddResourcesChangedListener(background.OnParentResourcesChanged); | |
} | |
} |
but it didn't work :/
it's why I did the same thing as, for example, Span does
maui/src/Controls/src/Core/Span.cs
Lines 12 to 18 in 5d67531
internal readonly MergedStyle _mergedStyle; | |
/// <include file="../../docs/Microsoft.Maui.Controls/Span.xml" path="//Member[@MemberName='.ctor']/Docs/*" /> | |
public Span() | |
{ | |
_mergedStyle = new MergedStyle(GetType(), this); | |
} |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Description of Change
I have implemented 2 solutions for this. I don't know which one is better, so please let me know and I will keep the one you prefer.
Issues Fixed
Fixes #19560
Screen.Recording.2024-01-28.at.01.51.32.mov