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

RadioButtons: Fix header not having correct color when disabled #3358

Conversation

Felix-Dev
Copy link
Contributor

@Felix-Dev Felix-Dev commented Oct 1, 2020

This PR fixes the RadioButtons' header not having the correct foreground color when the control is disabled. It now matches the foreground color seen for controls like the TextBox, ComboBox,....

This PR introduces the following new theme resources for the header foreground:

  • RadioButtonsHeaderForeground
  • RadioButtonsHeaderForegroundDisabled

These theme resources match the theme resources available for the TextBox header or the ComboBox header, for example.

In addition to the two theme resources above, I have also introduced the following new resources:

  • RadioButtonsTopHeaderMargin

Both the TextBox (TextBoxTopHeaderMargin) and the ComboBox (ComboBoxTopHeaderMargin) expose such a resource, thus the RadioButtons control should likely have one as well. I'm not exactly sold on the "Top" term here as the resource names to style the header foreground don't use that additional term and they appear totally fine to me... but for naming consistency, I went along here.

Motivation and Context

Fixes #3350

How Has This Been Tested?

Tested visually and added API test.

Screenshots:

Light Dark
image image

Open Questions

The ComboBox' Header also exposes a ComboBoxHeaderThemeFontWeight theme resource so customers can individually style the font weight of the header. I think exposing such a resource for the RadioButtons control would make sense as well, since setting the FontWeight API on the RadioButtons control affects both the header and the content of its radio buttons:

<muxc:RadioButtonsHeader="RadioButtonsHeader"
                   FontWeight="Bold">
    <x:String>Option 1</x:String>
    <x:String>Option 2</x:String>
</muxc:RadioButtons>

image

As written above, the ComboBox names that resource ComboBoxHeaderThemeFontWeight so if I were to stay consistent with the naming here, then it would be RadioButtonsHeaderThemeFontWeight. Again, not really sure why the "Theme" term is needed here...

The TextBox does not expose such a resource today but instead hard-codes the font weight of the header in the template:

Since the TextBox.FontWeight API already affects the TextBox's text, to allow individual customization of the header font weight, we should also introduce a theme resource here for the TextBox. Perhaps named TextControlHeaderThemeFontWeight (or TextBoxHeaderThemeFontWeight). Both the TextControl and the TextBox prefixes are used for for the TextBox's header resources.

Adding such a TextBox resource could also be done as part of this.

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Oct 1, 2020
@Felix-Dev Felix-Dev changed the title Radiobuttons: Fix header not having correct color when disabled RadioButtons: Fix header not having correct color when disabled Oct 1, 2020
Copy link
Contributor

@StephenLPeters StephenLPeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@StephenLPeters
Copy link
Contributor

If we were to include font wieght why wouldn't we also include font family and font size?

@StephenLPeters
Copy link
Contributor

@MikeHillberg can you weigh in about the font customizations?

@Felix-Dev
Copy link
Contributor Author

Felix-Dev commented Oct 1, 2020

If we were to include font wieght why wouldn't we also include font family and font size?

Yep, already thinking about them and writing a feature proposal as we speak since that would be a new addition for all the controls (TextBox/ComboBox/RadioButtons/...) using a header as far as I can tell on first look :) Didn't mention them in this PR because I viewed that slightly out of scope here.

@StephenLPeters
Copy link
Contributor

I think a feature proposal is appropriate and can be done independently of this work.

@StephenLPeters StephenLPeters added area-RadioButtons area-Styling team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Oct 1, 2020
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters merged commit 878ba35 into microsoft:master Oct 5, 2020
@Felix-Dev Felix-Dev deleted the user/Felix-Dev/radiobuttons-disabled-header-fix branch October 15, 2020 14:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RadioButtons: Header is not using the correct color when disabled
2 participants