Conversation
I have done some tests. If you change the system theme, when initializing a page we initialize Still missing to implement an event to detect change in appearance mode, right?. Is the PR in progress or are we going to divide the Spec into different PRs?. |
The failed tests seem unrelated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only reviewed the Core part, some minor remarks here and there
{ | ||
Unspecified, | ||
Light, | ||
Dark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we sure we won't need to extend this list ? if so, we might want to use strings...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@StephaneDelcroix Essentials defined it first in this way, so I think this was done to keep this in sync
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed follow the Essentials way for now. I'd like to think this is like the OS level theming and on top of that people can have their own custom theming, probably as an extension of this (naming will be a hell...). But if you implement your own theme as a user, you still might want to also follow the light and dark mode by the OS and show different colors based on that.
Xamarin.Forms.Core/AppThemeColor.cs
Outdated
|
||
namespace Xamarin.Forms | ||
{ | ||
public class AppThemeColor : BindableObject, IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this definitely doesn't look like a BindableObject. It's never parented, so it probably never gets a BindingContext set...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm using bindable properties? Should I be using something different?
|
||
namespace Xamarin.Forms | ||
{ | ||
public class OnAppTheme<T> : BindableObject, IDisposable |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not a BO. look at OnPlatform, OnDevice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, you might want a xaml markup language extension
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'm using bindable properties? Should I be using something different? And there is an extension as well; https://github.com/xamarin/Xamarin.Forms/pull/9958/files#diff-c65a7e8e0eed508c58cd0802b61b2cb1
public void Reload() | ||
{ | ||
foreach (var mr in MergedResources) | ||
OnValuesChanged(mr); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to avoid doing that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the latest changes it has at least changed to internal. I know it's not pretty and open to alternatives. But the problem is that the actual value doesn't really change. Depending on the theme the implicit operator will return a different color and that is why it's not picked up normally.
The only time this happens is when the theme changes on OS level and then a number of expensive operations already happen to redraw the whole screen already.
a52d012
to
226df64
Compare
5a85144
to
99e2860
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description of Change
Adds the cross-platform APIs to support different OS Themes
Background
See #9804 for the relevant APIs and how to use it. Because of timing I have removed the dependency on Essentials from this (see #9926) so we can get this merged in time for 4.6. After that, we can easily swap out this implementation for the Essentials one. By keeping naming and enum values, etc. the same this should be an easy task.
Issues Resolved
API Changes
Added:
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
I have added a gallery page which shows (nearly) all ways to use this new functionality. Check if that works as expected. Whenever the values are used as a
DynamicResource
they should update whenever the theme is changed while the app is running. Check out the gallery page or, to take it a step further, download the NuGets and take it for a spin in your own (sample) app.PR Checklist