Add the ability to change color programmatically #27

thadcodes · 2022-09-20T15:04:15Z

This should resolve issue #18, at least for the HarmonyColorPicker. It looks like the Classic one should already support programmatically updating itself.

ffgiraldez · 2022-09-23T10:01:12Z

Hi @thadcodes, sorry for the delay. I will look at those changes, but they look outstanding. Thanks.

riggaroo

Thanks so much for the contribution towards fixing the programmatic color change!

Two requests:

Can you do the brightness value change in a separate PR as its conflating the PR a bit.
The API needs to return a Hsv Color so as not to lose those individual properties, for instance the color black can be represented in many different HsvColors: Right now on launch, the color black is broken with this change as it doesn't allow the picker to move around on the palette. Also its defaulting on launch to have the whole color wheel as black, where as before it was brightness = 1
Thanks a lot!

riggaroo · 2022-09-25T19:08:32Z

...r-picker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorPicker.kt

+    harmonyMode: ColorHarmonyMode,
+    color: Color = Color.Red,
+    fixedBrightness: Float? = null,
+    onValueChanged: (Color) -> Unit


This shouldn't be changed to a Color at this point, the API should return a HsvColor otherwise color information is lost, as multiple HsvColors map to the same Color value. This will break for users who need individual color parts of information.

Makes sense. I did that because I wanted to make the input and value changed output match. So I'll create one where color: Color becomes hsvColor: HsvColor instead.

riggaroo · 2022-09-25T19:10:20Z

...r-picker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorPicker.kt

+        modifier = modifier,
+        harmonyMode = harmonyMode,
+        color = color,
+        onValueChanged = { HsvColor.from(it).let(onColorChanged) }


As mentioned in the other comment, this should be reverted as it'll lose specific color detail.

@riggaroo I believe I fixed this. Please let me know if that isn't the case

…matically and adds the ability to suppress the brightness bar with a fixed brightness value adjustment

thadcodes · 2022-09-26T14:26:10Z

@riggaroo Updated based on your feedback. Will open the brightness bar PR as soon as this is merged

ffranco-godaddy

Minimum change for LGTM, great work thanks.

ffgiraldez · 2022-09-30T10:37:07Z

README.md

-    onColorChanged = { hsvColor ->
-        currentColor.value = hsvColor.toColor()
-        extraColors.value = hsvColor.getColors(colorHarmonyMode = harmonyMode.value)
+    onValueChanged = { color ->


We must keep both HarmonyColorPicker and HarmonyColorPicker the same naming, onValueChange or onColorChange. I don't have a strong opinion about which one to choose.

I'm a little unclear, are you talking about the two HarmonyColorPicker functions or did you mean ClassicColorPickerScreen and HarmonyColorPicker needed to use the same naming?

If it's the two versions of HarmonyColorPicker I'll note the reason I changed from onColorChanged here to onValueChanged is because the passed in field name changed from color to value to keep the signatures for the Color old version and the new version that takes in HsvColor instead. I will note that the version with onColorChanged is deprecated with a replace with to make it easy to migrate to the new version for upgraders.

In case the issue is the inconsistency between classic and harmony I'm making a push that changes them to be consistent in the non-deprecated version

On second thought I just updated it to use the old naming for both. I think the better distinction between deprecated and non-deprecated is useful because it can help prevent someone from using the old one by mistake. But I don't feel anywhere near strongly enough to leave it that way

I'm a little unclear, are you talking about the two HarmonyColorPicker functions or did you mean ClassicColorPickerScreen and HarmonyColorPicker needed to use the same naming?

yes between ClassicColorPicker and HarmonyColorPicker a copy & paste error from my side sorry 😓

in this PR the non deprecated Composables are defined in this way

@Composable fun ClassicColorPicker( modifier: Modifier = Modifier, hsvColor: HsvColor = HsvColor.from(Color.Red), showAlphaBar: Boolean = true, onColorChanged: (HsvColor) -> Unit ) { @Composable fun HarmonyColorPicker( modifier: Modifier = Modifier, harmonyMode: ColorHarmonyMode, value: HsvColor = HsvColor.from(Color.Red), onValueChanged: (HsvColor) -> Unit ) {

We should keep the parameter names in sync.

On second thought I just updated it to use the old naming for both. I think the better distinction between deprecated and non-deprecated is useful because it can help prevent someone from using the old one by mistake. But I don't feel anywhere near strongly enough to leave it that way

Agree with that please update ClassicColorPicker to match HarmonyColorPicker with value/onValueChange.

ffgiraldez · 2022-09-30T14:17:51Z

README.md

-    onColorChanged = { hsvColor ->
-        currentColor.value = hsvColor.toColor()
-        extraColors.value = hsvColor.getColors(colorHarmonyMode = harmonyMode.value)
+    onValueChanged = { color ->


I'm a little unclear, are you talking about the two HarmonyColorPicker functions or did you mean ClassicColorPickerScreen and HarmonyColorPicker needed to use the same naming?

yes between ClassicColorPicker and HarmonyColorPicker a copy & paste error from my side sorry 😓

in this PR the non deprecated Composables are defined in this way

@Composable fun ClassicColorPicker( modifier: Modifier = Modifier, hsvColor: HsvColor = HsvColor.from(Color.Red), showAlphaBar: Boolean = true, onColorChanged: (HsvColor) -> Unit ) { @Composable fun HarmonyColorPicker( modifier: Modifier = Modifier, harmonyMode: ColorHarmonyMode, value: HsvColor = HsvColor.from(Color.Red), onValueChanged: (HsvColor) -> Unit ) {

We should keep the parameter names in sync.

On second thought I just updated it to use the old naming for both. I think the better distinction between deprecated and non-deprecated is useful because it can help prevent someone from using the old one by mistake. But I don't feel anywhere near strongly enough to leave it that way

Agree with that please update ClassicColorPicker to match HarmonyColorPicker with value/onValueChange.

ffgiraldez · 2022-09-30T14:23:05Z

...r-picker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorPicker.kt

+}
+
+/**
+ * Color Picker that programmatically updates.


[nit] this comment does not add too much value from a user perspective, the opposite of the comments in ClassicColorPicker.

I suggest removing it(as it was previously) or updating it like the ClassicColorPicker

thadcodes · 2022-09-30T14:27:36Z

Sorry @ffgiraldez I pushed my changes to the wrong branch that's why you are not seeing them, I'll have them up on the right branch.

I can see the argument on the Color Picker that programmatically updates. I actually wrote that when I was going to have two versions before just deprecating the old one. I am fine doing that as the comment should be in the deprecation warning now that they are not both correct versions

thadcodes · 2022-09-30T14:33:00Z

ok @ffgiraldez updated, sorry about the wasted review cycle. 🤦‍♂️

ffgiraldez · 2022-09-30T14:43:52Z

You probably will punch me, but please, could you update the example in the README to match real param names?

thadcodes · 2022-09-30T14:49:43Z

You probably will punch me, but please, could you update the example in the README to match real param names?

@ffgiraldez LOL no not at all, that's what I get for assuming IntelliJ would rename it here too and not checking. Fixed and pushed. Lucky you found that because I also had an error in the example that wasn't just renaming

ffgiraldez · 2022-09-30T14:53:42Z

@thadcodes LGTM now, I need to check why the signing is failing on CI😓

riggaroo · 2022-10-07T12:56:58Z

...r-picker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorPicker.kt

@@ -47,17 +72,16 @@ fun HarmonyColorPicker(
                .fillMaxHeight()
                .fillMaxWidth()
        ) {
-            val hsvColor = remember { mutableStateOf(HsvColor.from(color)) }
-            val updatedOnColorChanged by rememberUpdatedState(onColorChanged)
+            val color by rememberUpdatedState(color)


This is shadowing the color parameter. Maybe rename to updatedColor?

riggaroo · 2022-10-07T13:01:14Z

desktop/src/jvmMain/kotlin/HarmonyColorPickerScreen.kt

@@ -65,7 +67,7 @@ fun HarmonyColorPickerScreen() {
            harmonyMode = harmonyMode.value,
            modifier = Modifier.defaultMinSize(minHeight = 300.dp, minWidth = 300.dp),
            onColorChanged = { hsvColor ->


This doesn't seem to be compiling for desktop, is it working for you on desktop?

I see the issue and have pushed it @riggaroo. Sorry I didn't notice the desktop runner at all 🤦‍♂️

riggaroo

Thanks @thadcodes this is much better - its just not compiling on desktop, can you double check that it is for you?

… HarmonyColorPicker and fixed backwards compatibility case for the default value where it never updated the latest value

desktop/src/jvmMain/kotlin/HarmonyColorPickerScreen.kt

app/src/main/java/com/godaddy/android/colorpicker/ClassicColorPickerScreen.kt

riggaroo · 2022-10-09T09:09:10Z

@thadcodes Thanks - that is fixed! 🎉 One last request - can you update ClassicColorPickerScreen for the desktop build as that is using the deprecated version?

thadcodes · 2022-10-19T20:40:09Z

@thadcodes Thanks - that is fixed! 🎉 One last request - can you update ClassicColorPickerScreen for the desktop build as that is using the deprecated version?
@riggaroo sorry for the delayed response. This latest update should address your concerns

…PickerScreen.kt

riggaroo

Thanks @thadcodes! LGTM

ffgiraldez · 2022-10-24T11:47:24Z

Waiting for #49 to merge this. Sorry for all of this @thadcodes

riggaroo requested changes Sep 25, 2022

View reviewed changes

thadcodes closed this Sep 26, 2022

thadcodes force-pushed the main branch from 2b15ff6 to f888560 Compare September 26, 2022 13:50

thadcodes added 3 commits September 26, 2022 10:17

Should Resolve godaddy#18. Gives the ability to change values program…

815aacc

…matically and adds the ability to suppress the brightness bar with a fixed brightness value adjustment

PR Feedback

c870c39

Remove optional brightness bar

1dc8b89

thadcodes reopened this Sep 26, 2022

thadcodes changed the title ~~Add the ability to change color programmatically and remove brightness bar with fixed brightness value~~ Add the ability to change color programmatically Sep 26, 2022

ffranco-godaddy suggested changes Sep 30, 2022

View reviewed changes

ffgiraldez requested changes Sep 30, 2022

View reviewed changes

thadcodes requested review from riggaroo and ffgiraldez and removed request for riggaroo and ffgiraldez September 30, 2022 13:24

ffgiraldez reviewed Sep 30, 2022

View reviewed changes

Change classic and harmony to use old naming for parameters

fedffd9

thadcodes requested review from ffgiraldez and riggaroo and removed request for riggaroo and ffgiraldez September 30, 2022 14:33

Update readme, including fixing bad documentation

cd033e2

ffgiraldez approved these changes Sep 30, 2022

View reviewed changes

ffgiraldez requested review from ffranco-godaddy and removed request for ffranco-godaddy September 30, 2022 14:54

riggaroo reviewed Oct 7, 2022

View reviewed changes

Fix ambiguous function by changing removing default on new version of…

26c98ea

… HarmonyColorPicker and fixed backwards compatibility case for the default value where it never updated the latest value

thadcodes requested a review from riggaroo October 7, 2022 15:03

riggaroo reviewed Oct 9, 2022

View reviewed changes

desktop/src/jvmMain/kotlin/HarmonyColorPickerScreen.kt Outdated Show resolved Hide resolved

riggaroo reviewed Oct 9, 2022

View reviewed changes

app/src/main/java/com/godaddy/android/colorpicker/ClassicColorPickerScreen.kt Outdated Show resolved Hide resolved

Make classic color picker screen use new interface

c8ddd73

ffgiraldez requested a review from riggaroo October 19, 2022 21:31

JogahCR mentioned this pull request Oct 19, 2022

Make brightness bar optional #44

Merged

riggaroo added 2 commits October 20, 2022 15:47

Update app/src/main/java/com/godaddy/android/colorpicker/ClassicColor…

0eceb37

…PickerScreen.kt

Update desktop/src/jvmMain/kotlin/HarmonyColorPickerScreen.kt

3e9981f

riggaroo approved these changes Oct 20, 2022

View reviewed changes

Merge branch 'main' into main

2586532

ffgiraldez merged commit b86ca4b into godaddy:main Oct 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the ability to change color programmatically #27

Add the ability to change color programmatically #27

thadcodes commented Sep 20, 2022

ffgiraldez commented Sep 23, 2022

riggaroo left a comment

riggaroo Sep 25, 2022

thadcodes Sep 26, 2022

riggaroo Sep 25, 2022

thadcodes Oct 7, 2022

thadcodes commented Sep 26, 2022

ffranco-godaddy left a comment

ffgiraldez Sep 30, 2022

thadcodes Sep 30, 2022

thadcodes Sep 30, 2022

ffgiraldez Sep 30, 2022 •

edited

ffgiraldez Sep 30, 2022 •

edited

ffgiraldez Sep 30, 2022

thadcodes commented Sep 30, 2022

thadcodes commented Sep 30, 2022

ffgiraldez commented Sep 30, 2022

thadcodes commented Sep 30, 2022

ffgiraldez commented Sep 30, 2022

riggaroo Oct 7, 2022

riggaroo Oct 7, 2022

thadcodes Oct 7, 2022

riggaroo left a comment

riggaroo commented Oct 9, 2022

thadcodes commented Oct 19, 2022

riggaroo left a comment

ffgiraldez commented Oct 24, 2022

Add the ability to change color programmatically #27

Add the ability to change color programmatically #27

Conversation

thadcodes commented Sep 20, 2022

ffgiraldez commented Sep 23, 2022

riggaroo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thadcodes commented Sep 26, 2022

ffranco-godaddy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffgiraldez Sep 30, 2022 • edited

Choose a reason for hiding this comment

ffgiraldez Sep 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thadcodes commented Sep 30, 2022

thadcodes commented Sep 30, 2022

ffgiraldez commented Sep 30, 2022

thadcodes commented Sep 30, 2022

ffgiraldez commented Sep 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riggaroo left a comment

Choose a reason for hiding this comment

riggaroo commented Oct 9, 2022

thadcodes commented Oct 19, 2022

riggaroo left a comment

Choose a reason for hiding this comment

ffgiraldez commented Oct 24, 2022

ffgiraldez Sep 30, 2022 •

edited

ffgiraldez Sep 30, 2022 •

edited