-
Notifications
You must be signed in to change notification settings - Fork 24
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
Added a Color Wheel for Harmony Color Modes #14
Conversation
Properly scaling the magnifiers so they don't overlap the whole color wheel when much smaller. Fix position of magnifiers when content size was longer than it was wide.
import androidx.compose.foundation.layout.Column | ||
import androidx.compose.foundation.layout.height | ||
import androidx.compose.foundation.layout.padding | ||
import androidx.compose.material.* |
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.
[nit] Does your code style guide allow for wildcard imports?
Column(modifier = Modifier.fillMaxWidth()) { | ||
Text( | ||
modifier = Modifier.padding(16.dp), | ||
text = "a: ${currentColor.alpha} \n" + |
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.
[nit] This won't work for RTL languages
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.
Yeah this is just the sample app, not the library.
} | ||
HarmonyColorPicker( | ||
harmonyMode = harmonyMode.value, | ||
modifier = Modifier.size(400.dp), |
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.
[nit] I'm not comfortable with all these* hardcoded sizes. With good old views we defined sizes in terms of available space. Is this not an option here?
- height = 300 in the code above.
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 is just the sample app showing how you can set the size.
app/src/main/java/com/godaddy/android/colorpicker/SampleColorPickerActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/godaddy/android/colorpicker/SampleColorPickerActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/godaddy/android/colorpicker/SampleColorPickerActivity.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/godaddy/android/colorpicker/theme/Components.kt
Outdated
Show resolved
Hide resolved
...cker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorMagnifiers.kt
Outdated
Show resolved
Hide resolved
...cker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorMagnifiers.kt
Outdated
Show resolved
Hide resolved
...cker/src/commonMain/kotlin/com/godaddy/android/colorpicker/harmony/HarmonyColorMagnifiers.kt
Outdated
Show resolved
Hide resolved
Text(stringResource(R.string.classic_color_picker_sample)) | ||
}, | ||
navigationIcon = { | ||
BackButton { navController.navigateUp() } | ||
}) |
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.
[nit] Something's off with the formatting here
README.md
Outdated
@@ -33,13 +36,29 @@ Column { | |||
} | |||
``` | |||
|
|||
Or Add the `HarmonyColorPicker` to your Compose hierarchy for a HSV circle color wheel implementation: |
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.
[nit] 'HSV circle color wheel' seems a bit redundant. I'd remove 'circle'.
README.md
Outdated
}) | ||
``` | ||
|
||
The `HarmonyColorPicker` allows for you to set a certain `ColorHarmonyMode` to enable the different options to be |
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.
[nit] I'd rephrase this. It's not just 'options displayed', it's a whole new feature that deserves a proper description.
README.md
Outdated
|
||
### Harmony Mode | ||
|
||
To Change the harmony mode of the picker, pass in a different mode into the function: |
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.
[nit] Odd capitalization of 'Change'.
Added a Color Wheel for Harmony Color Modes
Added a Color Wheel for Harmony Color Modes
Added a new kind of color wheel for getting different
ColorHarmonyModes
.Usage
Screenshots
Android:
Desktop: