Skip to content
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

Use dynamic resources in Expander button theme. #14542

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

grokys
Copy link
Member

@grokys grokys commented Feb 8, 2024

What does the pull request do?

I'm not sure why, but the fluent Expander theme uses a mix of static and dynamic resources with seemingly no reason to differentiate them. It's not so much a problem for the resources in the default values for Expander as these can be overridden with local values, but for the toggle button and trigger styles, it means there's no way to customize these values.

Converted all of the StaticResources to DynamicResources.

Pinging @robloo who wrote this theme to check whether this change makes sense, or whether the static resources were there for a reason.

I'm not sure why, but the fluent `Expander` theme uses a mix of static and dynamic resources with seemingly no reason to differentiate them? It's not so much a problem for the resources in the default values for `Expander` as these can be overridden with local values, but for the toggle button and trigger styles, it means there's no way to customize these values.

Converted all of the `StaticResource`s to `DynamicResource`s.
@grokys grokys added bug backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Feb 8, 2024
@robloo
Copy link
Contributor

robloo commented Feb 8, 2024

@grokys The Fluent Theme for Expander started as a copy paste of upstream which also uses static resources several places.

https://github.com/microsoft/microsoft-ui-xaml/blob/winui2/main/dev/Expander/Expander.xaml

Over time we've converted some of then to dynamic as needed, above what had to change during the initial port.

There are a few reasons:

  1. Upstream uses static resources several places for performance reasons. The general thinking as I understand it is size-related properties (thickness, height, etc.) should be static and not changed at runtime. Brushes controlling color are full lightweight styling resources and can be changed at runtime. Or, any brushes that need to change on theme switch aren't static, everything else is a StaticResource.

  2. The bigger reason is WinUI doesn't even have DynamicResource it has StaticResoirce and ThemeResource. TheneResource is only re-evaluated on theme changes. This was a major decision for performance early in WinRT/UWP.

  3. For the port basically all ThemeResource usage was changed to DynamicResource and all others were kept the same.

So... the style was following upstream WinUI is basically the explanation here along with specifics noted above.

If you aren't concerned with the overhead of using DynamicResource for everything this PR is fine. Keep in mind Fluent theme basically moves everything possible out to a lightweight styling resource so there are a lot of them. Again, some things like thickness really don't change all that much.

Finally, I doubt Expander is the only theme ported from WinUI just changing ThemeResource to DynamicResource. I bet StaticResource comes up like this in other control themes.

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0044643-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@grokys
Copy link
Member Author

grokys commented Feb 8, 2024

The reason this came about was that I wanted to create a borderless Expander with no padding in the header area. Because ExpanderHeaderPadding is a static resource, I couldn't do that.

For the default values of Expander it doesn't really matter if they're static or dynamic because you can override them as local values, but for the toggle button in the header, they really need to be.

@maxkatz6 maxkatz6 merged commit a74a284 into master Feb 22, 2024
7 checks passed
@maxkatz6 maxkatz6 deleted the fixes/expander-dynamic-resource branch February 22, 2024 06:34
@maxkatz6 maxkatz6 added backported-11.0.x and removed backport-candidate-11.0.x Consider this PR for backporting to 11.0 branch labels Feb 22, 2024
maxkatz6 pushed a commit that referenced this pull request Feb 22, 2024
I'm not sure why, but the fluent `Expander` theme uses a mix of static and dynamic resources with seemingly no reason to differentiate them? It's not so much a problem for the resources in the default values for `Expander` as these can be overridden with local values, but for the toggle button and trigger styles, it means there's no way to customize these values.

Converted all of the `StaticResource`s to `DynamicResource`s.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants