fix: Update Android/iOS button styles to account for dark theme #424
Conversation
|
/cc @agneszitte-nventive |
| <Color x:Key="Color21">#FF7A7A7A</Color> | ||
| <Color x:Key="Color22">#FFE6E6E6</Color> | ||
| <ResourceDictionary x:Key="Light"> | ||
| <Color x:Key="AndroidBlue">#2196F3</Color> |
There was a problem hiding this comment.
We generally avoid naming keys with their actual color name, this avoids confusion when changed later on. Is it AndroidForeground instead? /cc @agneszitte-nventive
There was a problem hiding this comment.
@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?
There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
- +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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@Youssef1313 is there something to adjust here?
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
GitHub Issue (If applicable): The buttons part of #455
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 Runresults.Other information
Internal Issue (If applicable):