-
Notifications
You must be signed in to change notification settings - Fork 7
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
Copyright 2021 Splendo Consulting B.V. The Netherlands | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
package com.splendo.kaluga.resources | ||
|
||
class AndroidThemedColorProvider(light: String, dark: String): ThemedColorProvider() { | ||
|
||
/** The dark theme version of this color, already loaded */ | ||
override val darkColor = colorFrom(dark)!! | ||
|
||
/** The light theme version of this color, already loaded */ | ||
override val lightColor = colorFrom(light)!! | ||
|
||
override val color get() = if (isInDarkMode) darkColor else lightColor | ||
} | ||
|
||
actual fun themeColor(light: String, dark: String): ThemedColorProvider | ||
= AndroidThemedColorProvider(light, dark) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
/* | ||
Copyright 2021 Splendo Consulting B.V. The Netherlands | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
@file:JvmName("CommonThemedColorProvider") | ||
package com.splendo.kaluga.resources | ||
|
||
import kotlin.jvm.JvmName | ||
import kotlin.reflect.KProperty | ||
|
||
/** Multiplatform Color Provider that is aware of a Dark and Light mode */ | ||
abstract class ThemedColorProvider { | ||
|
||
/** light version of this color */ | ||
abstract val lightColor: Color | ||
|
||
/** dark version of this color */ | ||
abstract val darkColor: Color | ||
|
||
/** main color, automatically changing based on theme */ | ||
abstract val color: Color | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. 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. FYI: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I absolutely agree, but I don't see a shared implementation of |
||
} | ||
|
||
/** This extension allows to get the hexValue of the provided color with the "by" syntax */ | ||
inline operator fun ThemedColorProvider.getValue(thisRef: Any?, property: KProperty<*>): Color = color | ||
|
||
/** | ||
* One single color represented in 2 themes Dark or Light. | ||
* @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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually good point regarding There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. putting it in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, this is worth looking into and too bad I didn't think about it in the first place. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you stop tagging me please |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
/* | ||
Copyright 2021 Splendo Consulting B.V. The Netherlands | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
package com.splendo.kaluga.resources | ||
|
||
|
||
import platform.UIKit.UIColor | ||
import platform.UIKit.UIUserInterfaceStyle | ||
import platform.UIKit.colorWithDynamicProvider | ||
|
||
class IOSThemedColorProvider(light: String, dark: String): ThemedColorProvider() { | ||
|
||
override val lightColor = colorFrom(light)!! | ||
override val darkColor = colorFrom(dark)!! | ||
|
||
override val color = Color(UIColor.colorWithDynamicProvider { | ||
when (it?.userInterfaceStyle) { | ||
UIUserInterfaceStyle.UIUserInterfaceStyleDark -> darkColor | ||
else -> lightColor | ||
}.uiColor | ||
}) | ||
} | ||
|
||
actual fun themeColor(light: String, dark: String): ThemedColorProvider | ||
= IOSThemedColorProvider(light, dark) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
Copyright 2021 Splendo Consulting B.V. The Netherlands | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
package com.splendo.kaluga.resources | ||
|
||
class JsThemedColorProvider(light: String, dark: String): ThemedColorProvider() { | ||
|
||
/** The dark theme version of this color, already loaded */ | ||
override val darkColor = colorFrom(dark)!! | ||
|
||
/** The light theme version of this color, already loaded */ | ||
override val lightColor = colorFrom(light)!! | ||
|
||
override val color get() = if (isInDarkMode) darkColor else lightColor | ||
} | ||
|
||
actual fun themeColor(light: String, dark: String): ThemedColorProvider | ||
= JsThemedColorProvider(light, dark) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
/* | ||
Copyright 2021 Splendo Consulting B.V. The Netherlands | ||
|
||
Licensed under the Apache License, Version 2.0 (the "License"); | ||
you may not use this file except in compliance with the License. | ||
You may obtain a copy of the License at | ||
|
||
http://www.apache.org/licenses/LICENSE-2.0 | ||
|
||
Unless required by applicable law or agreed to in writing, software | ||
distributed under the License is distributed on an "AS IS" BASIS, | ||
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
See the License for the specific language governing permissions and | ||
limitations under the License. | ||
|
||
*/ | ||
|
||
package com.splendo.kaluga.resources | ||
|
||
class JvmThemedColorProvider(light: String, dark: String): ThemedColorProvider() { | ||
|
||
/** The dark theme version of this color, already loaded */ | ||
override val darkColor = colorFrom(dark)!! | ||
|
||
/** The light theme version of this color, already loaded */ | ||
override val lightColor = colorFrom(light)!! | ||
|
||
override val color get() = if (isInDarkMode) darkColor else lightColor | ||
} | ||
|
||
actual fun themeColor(light: String, dark: String): ThemedColorProvider | ||
= JvmThemedColorProvider(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.
I would rename these 2 properties just
light
anddark
, is already known they refer to color. What do you think?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.
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 simpleAlso, the third and more important variable is called
color
, sodarkColor
andlightColor
seem consistent to me.Good point, though