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

Adds ThemedColorProvider to resources module. #277

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

Conversation

posytive
Copy link
Contributor

@posytive posytive commented Apr 15, 2021

Draft for #336

Introduces the concept of a color that is aware of Dark/Light mode and it gives the proper value automatically.

Advantages:

  • the actual colors are not constantly re-created from the hex values, but they are chaced
  • on iOS side, colorWithDynamicProvider is used, which brings this functionality automatically
  • on Android side, the color value will not be cached but rather provided every time depending on the theme
  • if, for any reason, access to each theme value is still needed it remains possible by calling Provider.darkColor or Provider.lightColor

 Includes example usage in README
@codecov
Copy link

codecov bot commented Apr 15, 2021

Codecov Report

Merging #277 (4539f68) into master (54c37f7) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #277   +/-   ##
=========================================
  Coverage     49.54%   49.54%           
  Complexity      199      199           
=========================================
  Files            52       52           
  Lines          1314     1314           
  Branches        256      256           
=========================================
  Hits            651      651           
  Misses          530      530           
  Partials        133      133           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54c37f7...4539f68. Read the comment docs.

abstract val darkColor: Color

/** main color, automatically changing based on theme */
abstract val color: Color
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this API might be improved a bit, if color property type would be Flow. That would explicitly indicate its dynamic nature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see your point, and this feature can be implemented with a Flow.
But, thinking about the use case, I am not sure that is the best way to go.
This provider is meant to unify and simplify the usage of cross-platform color resources, when it comes to Dark/Light mode. Which is quite different in the way that each color on iOS has 2 versions, but not on Android (where a set of colors has 2 versions, not the color itself)
iOS has no need for a Flow, cause the changing of colors on UI is something that happens by using colorWithDynamicProvider . I don't see it as something you'd need to collect multiple times.

Also on the android side, I don't see a use case where is important to observe this color change, if not to trigger recomposition.
But even then, is not the change of color that should be observed but rather the Theme change.
So simplicity is preferred in this case.
In some projects using Kaluga, the common color resource file is currently made of getters or lazy initializations.

FYI:
With this idea, on Compose, there is no need to implement Material Colors: is currently intended for colors that we can use directly from a definition in common code. (a Compose Color can be easily created from a Kaluga Color).
Maybe it matters more to indicate the dynamic nature of these assets when referenced on the ViewModels.

Copy link
Contributor

Choose a reason for hiding this comment

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

But even then, is not the change of color that should be observed but rather the Theme change.

I absolutely agree, but I don't see a shared implementation of Flow<Theme> either. Moreover, in Compose UI it won't really work: the re-composition will indeed be triggered, but due to the skipping if the inputs haven't changed, it might not affect the places, where colors are being read (it really depends on the implementation).

Comment on lines +27 to +30
abstract val lightColor: Color

/** dark version of this color */
abstract val darkColor: Color
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename these 2 properties just light and dark, is already known they refer to color. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thought about it, but light and dark are also the name of the params which are strings.
I preferred not to rename those to lightHexValue or ligthValue, but to rename these instead.
Because I prefer to call it with val somename by themeColor(light = "#fff", dark = "#000"): very simple
Also, the third and more important variable is called color, so darkColor and lightColor seem consistent to me.

Good point, though

* @param light color when presented in Light mode: formatted as hexadecimal string
* @param dark color when presented in Dark mode: formatted as hexadecimal string
*/
expect fun themeColor(light: String, dark: String): ThemedColorProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an isInDarkMode method on the kaluga level. So I dont see the point of making this platform specific. It should just be:

fun themeColor(light: Color, dark: Color): Color {
if (isInDarkMode) dark else light
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if you want the dynamic behaviour on Android on a named level, you can just make a values_night folder and put the dark color there.

Copy link
Contributor

Choose a reason for hiding this comment

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

The main reason that simple if statement doesn't reflect change in real time on iOS (including SwiftUI Previews) so UIColor should be created on other way using special constructor to have dynamic colors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, look at the implementation. iOS provides a color that is both light and dark, whereas this does not exist on Android. See https://splendo.slack.com/archives/CN1T4SM5Y/p1618294377023400

Copy link
Contributor

@avdyushin avdyushin Apr 29, 2021

Choose a reason for hiding this comment

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

Actually good point regarding values-night worth to try on Android and just returning same color name to be handled by Android itself?

Copy link
Collaborator

Choose a reason for hiding this comment

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

putting it in values-night defies the point of what this method tries to do, defining colors once in common code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually good point regarding values-night worth to try on Android and just returning same color name to be handled by Android itself?

Yes, this is worth looking into and too bad I didn't think about it in the first place.
I was doubting that values-night is the folder used in case of dark mode (earlier versions on Android would use this folder for other modes), but indeed it is.

I will make some more experiments when I have time

@Tijl I agree that the preferred usage of this class is indeed by using the strings directly.
Although the problem is trying to solve is also to unify the usage of colors on platform without breaking the dark/light switch.
In that sense, I think is worth looking into the values-night solution while still using these ColorProviders, we would only need to add 1 extension to accepts colors directly. It will still be valuable the fact that we define the difference in the constructor.
I think this is useful for project where the best/only solution to branded colors is with scripts that generates the XML files in the correct places.
In general, we should consider how to support projects that already have their color definitions in xml and don't need to change that, while they do need to add support fro dark/light easily
(imho)

Copy link

Choose a reason for hiding this comment

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

Can you stop tagging me please

Copy link
Contributor

@Daeda88 Daeda88 left a comment

Choose a reason for hiding this comment

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

This doesnt look like it solves anything really. And if it does, it does so in an unclear way.

@corrado4eyes corrado4eyes added the 0.2.0 expected in the 0.2.0 release label May 10, 2021
@posytive
Copy link
Contributor Author

This doesnt look like it solves anything really. And if it does, it does so in an unclear way.

Fair point, this could be more complete to be used in an easier way, especially on Android.
But I did change the README, and tried to explain there, the best I could, what this is for. (Dark/Light switch).
So please let me know what is not clear about that.
Otherwise my answer is This comment is not helpful really. And if it does, it does so in an unclear way. :)

Truth be told, the way I am currently using it in a project, does include a piece of glue that makes it more usable on android:

@Composable
fun ThemedColorProvider.composeThemedColor(): Color
    = if (isSystemInDarkTheme()) Color(darkColor) else Color(lightColor)

So, when I need an automatically recomposing color on Android, I'd go Colors.backgroundProvider.composeThemedColor() INSTEAD of using MaterialTheme.

Even better if I can directly use Compose's isSystemInDarkTheme() instead of Kaluga's isInDarkMode in the actual class on this PR.
I think it will look more complete then, but I need to change the target branch and consider this an RC feature.
I'll do that as soon as I have time for it

@thoutbeckers thoutbeckers removed the 0.2.0 expected in the 0.2.0 release label Jul 20, 2021
@thoutbeckers
Copy link
Collaborator

@posytive this still has no linked issue in the description

@thoutbeckers thoutbeckers marked this pull request as draft August 26, 2024 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants