-
Notifications
You must be signed in to change notification settings - Fork 752
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
chore: Cleanup DependencyPropertyValuePrecedences #15684
base: master
Are you sure you want to change the base?
chore: Cleanup DependencyPropertyValuePrecedences #15684
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.
This should be the first file to review in the PR.
- We were not using
TemplatedParent
- We were using
ExplicitStyle
for both explicit and implicit styles. - We were using
ImplicitStyle
for default styles (as indicated in the deleted remarks section of the doc comment) - We were not using
DefaultStyle
8961524
to
af8b5f2
Compare
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15684/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15684/index.html |
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15684/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15684/index.html |
Let's let @jeromelaban to confirm, but it seems safe |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15684/index.html |
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15684/index.html |
🤖 Your WebAssembly Sample App stage site is ready! Visit it here: https://unowasmprstaging.z20.web.core.windows.net/pr-15684/index.html |
Pushing to Uno 6 |
@@ -345,7 +345,7 @@ void SetDefaultForeground(DependencyProperty foregroundProperty) | |||
{ | |||
// Hyperlink doesn't appear to inherit foreground from the parent. | |||
// So, we set this with ImplicitStyle precedence which is a higher precedence than Inheritance. | |||
this.SetValue(foregroundProperty, DefaultTextForegroundBrush, DependencyPropertyValuePrecedences.ImplicitStyle); | |||
this.SetValue(foregroundProperty, DefaultTextForegroundBrush, DependencyPropertyValuePrecedences.DefaultStyle); |
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.
The right way to do that is by using this.ForegroundProperty.OverrideMetadata()
.
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.
@carldebilly Do you mean to remove the inherits flag? OverrideMetadata will "combine"/"merge" the flags, so it won't work :(
Unless we introduce another overload OverrideMetadata(bool doNotMergeFlags)
or so.
We'll probably need to understand how WinUI does this and do the same.
This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or it will be closed in 10 days. |
Definitely still needed |
🤖 Your Docs stage site is ready! Visit it here: https://unodocsprstaging.z13.web.core.windows.net/pr-15684/index.html |
Closes #16356
Removes an unused precedence and renames two others to match their actual semantics.
Note: the removal of the unused precedence makes the current precedences count 8, which means the
DependencyPropertyDetails.Stack
arrays will now be of length 8 instead of 16 :)PR Type
What kind of change does this PR introduce?
What is the current behavior?
What is the new behavior?
PR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Run
results.Other information
Internal Issue (If applicable):