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

[v2] Style improvements #210

Merged
merged 7 commits into from
Oct 17, 2022
Merged

[v2] Style improvements #210

merged 7 commits into from
Oct 17, 2022

Conversation

amwx
Copy link
Owner

@amwx amwx commented Oct 14, 2022

Some updates to the styles here, including a big one that should help on non-Windows platforms:

On account of #128 & #209, I'm adding a way to help adjust text positioning in some controls when not on Windows. Right now we have a problem that Fluent design is primarily targeted at Windows where Segoe UI or Segoe UI Variable font families are available, leading to some controls having text positioned wrong on Mac/Linux where the fonts aren't available. This is done because fluent design says the first line of text should be level with the control (accounting for multi-line scenarios) so vertical alignment is set to top and margins/paddings are used to position the text - based on Segoe UI. The worst offenders of this were CheckBox, RadioButton, and ComboBox.

To try and fix this, I've added a property to FluentAvaloniaTheme UseStyleOverridesForNonWindows, which defaults to true. If set, and not on Windows, some styling overrides are injected to use vertical alignment and adjust some of the uneven margins to make things look a little better. These overrides are added internally inside FATheme, so you can still override further if needed. This isn't a perfect solution because this may lead to different appearances in Windows vs other platforms (primarily if multi-line text is needed), but this is as much as I can do on my end at this point and I think its a fair solution

I've only addressed the 3 controls above. The rest by my eye look the same on Windows or aren't affected.

[WSL2 ubuntu linux]
Before:
image

After:
image

[Other]

  • [Improvement] Change padding of CheckBox to use DynamicResource instead of StaticResource
  • [Improvement] Update Padding of ListBoxItem to match WinUI (left margin increased from 12 to 16)
  • [Cleanup] Removed the old DataGrid axaml file that I forgot to delete
  • [Cleanup] Removed some unused resources from the theme resource dictionaries. These were all marked in "Legacy Brushes" section and/or had the "ThemeBrush" suffix in the name. These are unused in WinUI and I don't know why MS still has them in the Fluentv2 styles, but we don't need them so they're gone
  • [Fix] Updated the expander style to improve template and fix issue where expander header was rounded on all sides even when expanded

fixes #209
fixes #128

Edit:
I've changed the API a little. Instead of a bool and only applying on non-Windows, it's now an enum based on @robloo's suggestion below, and the property is now called TextVerticalAlignmentOverrideBehavior. The default value is EnabledNonWindows.

public enum TextVerticalAlignmentOverride
{
    Disabled, // never use the overrides
    EnabledNonWindows, // only use when not on Windows
    AlwaysEnabled, // always use the overrides
}

@amwx amwx added enhancement New feature or request v2 Related to v2 styling labels Oct 14, 2022
@robloo
Copy link
Contributor

robloo commented Oct 15, 2022

Nice to see these changes!

My only concern here is for apps that want to ensure the same uniform look on all platforms. In that case they may want to use the override styles on all platforms including windows (I would probably use that as the default myself). In that case I wonder if the option shouldn't be renamed to something more specific like ForceVerticalAlignmentToCenterOfText and then support three states Disabled, EnabledNonWindows, Enabled (last for all platforms).

@robloo
Copy link
Contributor

robloo commented Oct 15, 2022

These were all marked in "Legacy Brushes" section and/or had the "ThemeBrush" suffix in the name. These are unused in WinUI and I don't know why MS still has them in the Fluentv2 styles

They were kept in for compatibility to not break existing apps so much with the changes for Fluent v2.

@amwx
Copy link
Owner Author

amwx commented Oct 15, 2022

Nice to see these changes!

My only concern here is for apps that want to ensure the same uniform look on all platforms. In that case they may want to use the override styles on all platforms including windows (I would probably use that as the default myself). In that case I wonder if the option shouldn't be renamed to something more specific like ForceVerticalAlignmentToCenterOfText and then support three states Disabled, EnabledNonWindows, Enabled (last for all platforms).

I had a similar thought, just thinking of the best approach. I like the enum idea though, might be what I end up doing. I need to think on the property name some though

@amwx amwx merged commit 0920bd7 into master Oct 17, 2022
@amwx amwx deleted the v2_StylingImprovements branch October 17, 2022 22:24
@amwx amwx mentioned this pull request Nov 9, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request styling v2 Related to v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

In ComboBox the value is shifted vertically macOS Support
2 participants