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.

Sign up for GitHub

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

fix: Update Android/iOS button styles to account for dark theme #424

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: Update Android/iOS button styles to account for dark theme #424

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions src/Uno.Playground.Shared/DefaultStyles.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@
<Style x:Key="AndroidButtonStyle"
TargetType="Button">
<Setter Property="Background"
Value="#2196f3" />
Value="{ThemeResource AndroidBlue}" />
<Setter Property="Foreground"
Value="White" />
<Setter Property="Padding"
Expand Down Expand Up @@ -860,7 +860,7 @@
<ObjectAnimationUsingKeyFrames Storyboard.TargetName="ContentPresenter"
Storyboard.TargetProperty="Foreground">
<DiscreteObjectKeyFrame KeyTime="0"
Value="#42000000" />
Value="{ThemeResource AndroidForegroundDisabled}" />
</ObjectAnimationUsingKeyFrames>
</Storyboard>
</VisualState>
Expand Down Expand Up @@ -894,7 +894,7 @@
<Setter Property="Background"
Value="Transparent" />
<Setter Property="Foreground"
Value="#007aff" />
Value="{ThemeResource iOSBlue}" />
<Setter Property="Padding"
Value="0" />
<Setter Property="Height"
Expand Down Expand Up @@ -933,7 +933,7 @@
<ObjectAnimationUsingKeyFrames Storyboard.TargetName="ContentPresenter"
Storyboard.TargetProperty="Foreground">
<DiscreteObjectKeyFrame KeyTime="0"
Value="#33000000" />
Value="{ThemeResource iOSForegroundDisabled}" />
</ObjectAnimationUsingKeyFrames>
</Storyboard>
</VisualState>
Expand Down
59 changes: 36 additions & 23 deletions src/Uno.Playground.Shared/Styles/StyleGuide/Colors.xaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,32 +4,45 @@

<!-- Color Resources -->
<ResourceDictionary.ThemeDictionaries>
<ResourceDictionary x:Key="Default">
<Color x:Key="Color01">#FFFFFFFF</Color>
<Color x:Key="Color02">#FF000000</Color>
<Color x:Key="Color03">#FF159BFF</Color>
<Color x:Key="Color04">#FF242424</Color>
<Color x:Key="Color05">#80242424</Color>
<Color x:Key="Color06">#FF313131</Color>
<Color x:Key="Color07">#FF7A67F8</Color>
<Color x:Key="Color08">#FF67E5AD</Color>
<Color x:Key="Color09">#FFCED0D3</Color>
<Color x:Key="Color10">#FF919191</Color>
<Color x:Key="Color11">#FFF85977</Color>
<Color x:Key="Color12">#FF9B9B9B</Color>
<Color x:Key="Color13">#FF229DFC</Color>
<Color x:Key="Color14">#FFF65678</Color>
<Color x:Key="Color15">#FF6CE5AE</Color>
<Color x:Key="Color16">#FF7A69F5</Color>
<Color x:Key="Color17">#FFD8D8D8</Color>
<Color x:Key="Color18">#1A159BFF</Color>
<Color x:Key="Color19">#FF272C35</Color>
<Color x:Key="Color20">#FFEEEEEE</Color>
<Color x:Key="Color21">#FF7A7A7A</Color>
<Color x:Key="Color22">#FFE6E6E6</Color>
<ResourceDictionary x:Key="Light">
<Color x:Key="AndroidBlue">#2196F3</Color>
Copy link
Member

Choose a reason for hiding this comment

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

We generally avoid naming keys with their actual color name, this avoids confusion when changed later on. Is it AndroidForeground instead? /cc @agneszitte-nventive

Copy link
Contributor

Choose a reason for hiding this comment

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

@jeromelaban / @Youssef1313 / @kazo0 / @NVLudwig
First, should we not have proper Light/Dark ResourceDictionnary for all the colors for Uno.Playground?

@kazo0 regarding the android/ios buttons here, does it still make sense that we have AndroidButtonStyle, iOSButtonStyle in the Playground DefaultStyles.xaml ?
Should we not use the Themes lib instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

@agneszitte I agree we shouldn't have these styles in Playground.

In this case we will need to update the sample itself (I don't know where those samples are maintained)

Copy link

Choose a reason for hiding this comment

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

  • +1 for color-name as name; you should describe "what it is used for"(purpose) than "what it is"(color).
    use this syntax: {Qualifier}{Control}{Resource}{State?}
    eg: AndroidButtonBackground, IosButtonBackgroundDisabled // pick your poison: Ios or iOS
  • I would keep them inside DefaultStyles (which houses the control style), those colors are closely related to the specific control style.
  • It is, more or less, fine that Color\d{2} are not theme-dependent. However, I would fear that some shared samples could be relying on these resources to be specific color. I would be hesitant to touch them now.

Copy link

Choose a reason for hiding this comment

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

on the bright side, the control styles in DefaultStyles or Styles\Controls\* all seem to be x:Key-ed
so changing them should be less of a problem

Copy link
Contributor

@kazo0 kazo0 Apr 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

For the AndroidButtonStyle and iOSButtonStyle, they cant necessarily just be replaced with the styles coming from Themes. The default Android button from a vanilla native Android app doesn't match a Material Design 3 Button style. I believe these styles are here to mimic default styles from the native iOS/Android SDKs

Copy link
Member

Choose a reason for hiding this comment

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

@Youssef1313 is there something to adjust here?

Copy link
Member Author

Choose a reason for hiding this comment

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

On a second thought, I think we should really remove all these styles. The expectation should be that code in Playground matches behavior when copied into an Uno Wasm app right? But that is not the case with Playground defining own styles and overriding some control templates, etc.

The real fix would be to remove all these and also update the Playground snippets (needs someone with access to the API).

I'm not sure though if we can merge this PR until we have the proper fix (assuming you agree that it's the proper fix)

Copy link
Member Author

Choose a reason for hiding this comment

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

When updating the snippets, we also don't want to include any explicit colors because that will work on either light or dark but likely not both.

<Color x:Key="AndroidForegroundDisabled">#42101010</Color>

<Color x:Key="iOSBlue">#007AFF</Color>
<Color x:Key="iOSForegroundDisabled">#33787880</Color>
</ResourceDictionary>
<ResourceDictionary x:Key="Dark">
<Color x:Key="AndroidBlue">#1976D2</Color>
<Color x:Key="AndroidForegroundDisabled">#42767676</Color>

<Color x:Key="iOSBlue">#0A84FF</Color>
<Color x:Key="iOSForegroundDisabled">#5B787880</Color>
</ResourceDictionary>
</ResourceDictionary.ThemeDictionaries>

<Color x:Key="Color01">#FFFFFFFF</Color>
<Color x:Key="Color02">#FF000000</Color>
<Color x:Key="Color03">#FF159BFF</Color>
<Color x:Key="Color04">#FF242424</Color>
<Color x:Key="Color05">#80242424</Color>
<Color x:Key="Color06">#FF313131</Color>
<Color x:Key="Color07">#FF7A67F8</Color>
<Color x:Key="Color08">#FF67E5AD</Color>
<Color x:Key="Color09">#FFCED0D3</Color>
<Color x:Key="Color10">#FF919191</Color>
<Color x:Key="Color11">#FFF85977</Color>
<Color x:Key="Color12">#FF9B9B9B</Color>
<Color x:Key="Color13">#FF229DFC</Color>
<Color x:Key="Color14">#FFF65678</Color>
<Color x:Key="Color15">#FF6CE5AE</Color>
<Color x:Key="Color16">#FF7A69F5</Color>
<Color x:Key="Color17">#FFD8D8D8</Color>
<Color x:Key="Color18">#1A159BFF</Color>
<Color x:Key="Color19">#FF272C35</Color>
<Color x:Key="Color20">#FFEEEEEE</Color>
<Color x:Key="Color21">#FF7A7A7A</Color>
<Color x:Key="Color22">#FFE6E6E6</Color>

<!-- SolidColorBrush Resources -->
<SolidColorBrush x:Key="Color01Brush"
Color="{ThemeResource Color01}" />
Expand Down