Create requiresLightText util #311
Conversation
src/services/colors/color_utils.js
Outdated
| * be light or dark for better readability. | ||
| * The color must be specified via its red, green and blue value in the range of | ||
| * 0 to 255. | ||
| * The formular is based on this Stackoverflow answer: https://stackoverflow.com/a/3943023 |
There was a problem hiding this comment.
Typo: formular -> formula
src/services/colors/color_utils.js
Outdated
| * @param {number} blue The blue component in the range 0 to 255 | ||
| * @returns {boolean} True if the background requires light text on it, false otherwise. | ||
| */ | ||
| function requiresLightText(red, green, blue) { |
There was a problem hiding this comment.
Can we rename this function isColorDark? This way the function name is specific to the input, without any presumptions about how it's being used.
And how about we do some ducktyping with the params to support a hex value input? We can use the conversion functions from https://stackoverflow.com/questions/5623838/rgb-to-hex-and-hex-to-rgb.
function isColorDark(redOrHex, green, blue) {
let [r, g, b];
if (typeof redOrHex === 'string') {
[r, g, b] = hexToRgb(redOrHex);
} else {
[r, g, b] = [red, green, blue];
}
/* ... */
}There was a problem hiding this comment.
We'd have to update the tests to reflect this change.
There was a problem hiding this comment.
How strong are your feelings for using the red or hex. I would rather keep this method as simple as possible. Also currently we don't need this with hex. So I would rather implement it if we really need it (to not produce dead code right from the beginning).
src/services/colors/color_utils.js
Outdated
| return luminance <= 0.179; | ||
| } | ||
|
|
||
| export { requiresLightText }; |
There was a problem hiding this comment.
Can we rename this file to is_color_dark.js to match the function name? And can we name the folder color (singular)?
There was a problem hiding this comment.
I didn't create that folder, so I won't rename it in this PR, since it means updating a lot of other places, too, what would be unrelated to this PR. But I don't mind if you want to rename it in another PR :D
|
1/10. We can skip it for now. :)
…On Wed, Jan 17, 2018 at 8:19 AM Tim Roes ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/services/colors/color_utils.js
<#311 (comment)>:
> @@ -0,0 +1,22 @@
+/**
+ * This function calculates if the text on a given background color needs to
+ * be light or dark for better readability.
+ * The color must be specified via its red, green and blue value in the range of
+ * 0 to 255.
+ * The formular is based on this Stackoverflow answer: https://stackoverflow.com/a/3943023
+ * which itself is based upon the WCAG recommendation for color contrast.
+ *
+ * @param {number} red The red component in the range 0 to 255
+ * @param {number} green The green component in the range 0 to 255
+ * @param {number} blue The blue component in the range 0 to 255
+ * @returns {boolean} True if the background requires light text on it, false otherwise.
+ */
+function requiresLightText(red, green, blue) {
How strong are your feelings for using the red or hex. I would rather keep
this method as simple as possible. Also currently we don't need this with
hex. So I would rather implement it if we really need it (to not produce
dead code right from the beginning).
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#311 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ABLmg6nAqxqOKkHRPPc2l6fKZRJ1oW1iks5tLh19gaJpZM4RhMjI>
.
|
This PR introduces a util function to calculate whether we need to draw light or dark text on a specific background color. We need this in visualization, so we can determine what text color we should use on specific background colors for metrics and heatmap.
Since I think this function can be of value for other teams too and it's related to design, I think the EUI services are a good place to implement it.
The forumlar to calculate luminance is described in this stackoverflow answer ans is based on the WCAG guidelines.
For the case anyone wonders what the colors in the tests are: those are our visualization colors from
visualization_colors.jsand some random colors from the color palettes uses in visualizations at the moment.