feat: dynamic colors - #864
Conversation
0ff05b9 to
764fc28
Compare
|
Colors now match screenshots in description. Attempted a slightly different approach previously that should have been reverted prior to publishing this CR as a draft. |
There was a problem hiding this comment.
What is the point of this file? If we are going to merge it, we intend to support it. I would prefer you remove this.
There was a problem hiding this comment.
Can we use Material Components attributes here instead? If you are unwilling to switch to Material colors, I can potentially do that. I would just prefer, if we are updating the theming engine, then maybe we should update to Material 3 as well?
There was a problem hiding this comment.
I'll explore a larger migration/refactor soon, as mentioned in #864 (comment).
There was a problem hiding this comment.
Same as above, not needed.
|
|
||
| import android.app.Activity | ||
| import android.content.Intent | ||
| import android.os.Build |
There was a problem hiding this comment.
Is this import needed?
There was a problem hiding this comment.
Ah, I missed cleaning that up from the initial use of https://developer.android.com/reference/android/os/Build.VERSION.
Thanks for catching it!
| val dynamicColorsSupported: Boolean | ||
| get() = context.resources.getBoolean(R.bool.dynamic_colors_supported) | ||
|
|
There was a problem hiding this comment.
I don't think supported should matter based on a config, rather it should matter based on android version.
There was a problem hiding this comment.
Initially I had this as an OS version check, but this approach felt more natural to me.
My reasoning is that we need new SDK v31 (Android 12) theme files for Dynamic Color support, and they get automatically preferred based on the OS version. So by having this bool file, the OS level determines which resource files to use and this comes along with it. This boolean will be available everywhere that the values are.
I do see your point about the file just containing a boolean though. Wrapping a version check in a properly named function could probably also be sufficient.
I'm happy to go either way if you have a strong preference.
There was a problem hiding this comment.
I think the version check would be better, as that is the pattern in the rest of the codebase, and is more idiomatic to Android. Nice thought though.
| android.R.color.system_accent1_600, | ||
| android.R.color.system_accent1_200, |
There was a problem hiding this comment.
Is it possible not to hardcode these?
There was a problem hiding this comment.
I can see if there is a more natural way to handle this when exploring the Material 3 migration.
| android.R.color.system_neutral1_10, | ||
| android.R.color.system_neutral1_900, | ||
| isNight | ||
| ) |
There was a problem hiding this comment.
Is it possible to not hardcode these?
No problem, thank you for the thoughtful review! I considered this before starting, and my thought was that this could be an incremental step to full Material 3 and/or Material 3 Expressive support. I didn't look into that very thoroughly though, and focused on fitting this into the existing codebase. I'll be out of town for a couple of days, but I'd be happy to look into a migration once I'm back. We can see how large the scope is and go from there. |
2 participants
Revision 1 Changes
Why
Testing
Related Links