Add alpha channel support #15

Hawkbat · 2021-07-17T21:52:03Z

Checklist

Code is up-to-date with the master branch.
yarn test (npm run test) passes with this change.
The new commits follow conventions explained in CONTRIBUTING.md

Description of Change

I took a crack at adding alpha support. The only public interface changes are the addition of an optional boolean alpha prop to display the alpha slider and toggle parsing of alpha values from the text inputs, as well as an extension of the color constructors to accept 4/8-digit hex strings, an additional a property on rgb/hsv colors, and four-value arrays.

There should be no breaking changes unless a user was already passing in 4/8-digit hex strings, four-value arrays, or objects with a alpha channels, in which case those will now be round-tripped appropriately instead of truncated. Since that behavior would technically be a violation of the previous interface I think it's up to you how you want to handle that.

The utils are updated to handle the alpha channel if present/not undefined. There's a new alpha slider component below the hue slider that's only visible if the alpha prop is passed in. And the text inputs will correctly parse and display alpha values if they are provided. If no alpha value is set, the components return the colors without an alpha value.

Absolutely willing to go back and forth on this if it's not up to snuff for the project! I'd been looking for a TypeScript-based color picker for a while and couldn't find one quite as clean as yours.

Add alpha slider component Add alpha props and alpha slider to components

Wondermarin

Hi!
Thank you for your work! I will try to review it asap.

However, I'm not sure about the possible undefined in a (in the ColorRGB and ColorHSV interfaces), when user does not use the alpha channel. Maybe it would be better to leave it equal to 100?

Hawkbat · 2021-07-18T21:31:45Z

I'd be concerned that existing users using regular non-alpha colors might be accidentally impacted by the picker suddenly returning alpha values; for example if ColorRGB.a defaults to 255 then gets converted into a hex string before returning to the end user it would be an eight-digit hex string ending in ff instead of the six-digit hex string it is now. This way the 4-channel results are only introduced if the alpha prop is present and/or the a property is present. The alternative would be to introduce a breaking change where alpha values and eight-digit hex strings are always returned even if the alpha prop isn't present, which seems very risky to me.

Wondermarin

I'd be concerned that existing users using regular non-alpha colors might be accidentally impacted by the picker suddenly returning alpha values; for example if ColorRGB.a defaults to 255 then gets converted into a hex string before returning to the end user it would be an eight-digit hex string ending in ff instead of the six-digit hex string it is now. This way the 4-channel results are only introduced if the alpha prop is present and/or the a property is present. The alternative would be to introduce a breaking change where alpha values and eight-digit hex strings are always returned even if the alpha prop isn't present, which seems very risky to me.

Hmm. I think you're right.

Don't you think it would be more correct to convert the alpha channel to values from 0 to 1, instead of from 0 to 255 (ColorRGB) and from 0 to 100 (ColorHSV). Since in css when using rgba, browser expects to get 1 as 100% for the alpha channel. Then the users will not need to convert it.

Wondermarin · 2021-07-19T13:43:26Z

src/components/Alpha.component.tsx

+  };
+
+  return (
+    <Interactive className="rcp-alpha" onChange={updateColor}>


I think you need to change color of the alpha channel to the selected color.

<Interactive style={{ background: `linear-gradient(to right, rgba(${color.rgb.r}, ${color.rgb.g}, ${color.rgb.b}, 0), rgba(${color.rgb.r}, ${color.rgb.g}, ${color.rgb.b}, 1)) top left / auto auto, conic-gradient(#666 0.25turn, #999 0.25turn 0.5turn, #555 0.5turn 0.75turn, #999 0.75turn) top left / 12px 12px repeat`, }} className="rcp-alpha" onChange={updateColor} >

Wondermarin · 2021-07-19T13:44:54Z

src/components/Alpha.component.tsx

+      <div
+        className="rcp-alpha-cursor"
+        style={{ left: position, backgroundColor: `hsl(0, 0%, ${color.hsv.a ?? 100}%)` }}
+      />


For the cursor, respectively, also.

<div className="rcp-alpha-cursor" style={{ left: position, background: `linear-gradient(to right, rgba(${color.rgb.r}, ${color.rgb.g}, ${color.rgb.b}, ${color.rgb.a}), rgba(${color.rgb.r}, ${color.rgb.g}, ${color.rgb.b}, ${color.rgb.a})) top left / auto auto, conic-gradient(#666 0.25turn, #999 0.25turn 0.5turn, #555 0.5turn 0.75turn, #999 0.75turn) top left / 12px 12px repeat`, }} />

Wondermarin · 2021-07-19T13:46:31Z

src/css/styles.css

+  background: linear-gradient(to right, rgba(255, 255, 255, 0), rgba(255, 255, 255, 1)) top left / auto auto,
+    conic-gradient(#666 0.25turn, #999 0.25turn 0.5turn, #555 0.5turn 0.75turn, #999 0.75turn) top left / 12px 12px
+      repeat;


Because we are now changing the color dynamically. background is no longer needed.

Wondermarin · 2021-07-19T13:48:15Z

src/utils/convert.util.ts

+    let newValue = "#";
+
+    for (let i = 1; i < value.length; i++) {
+      newValue += value[i] + value[i];
+    }
+
+    return newValue;


What do you think about this change? It does not create variables internally and is more concise in writing.

value = value .split("") .map((v, i) => (i ? v + v : "#")) .join(""); return value;

Wondermarin · 2021-07-19T13:49:57Z

src/utils/convert.util.ts

+  if (!Number.isNaN(a)) {
+    return { r, g, b, a };
+  }


Here the condition is not necessary, because our interface resolves a as undefined.

This one I think we actually do need to keep, or at least something similar; a in this case is the result of calling parseInt(hex.slice(6, 8), 16). parseInt() returns NaN if it cannot parse the value. So when calling this function with a six-digit hex string like #FF007F, the parseInt() will return NaN and you'll end up with { r: 255, g: 0, b: 127, a: NaN } which is probably not what we want.

Wondermarin · 2021-07-19T13:50:32Z

src/utils/convert.util.ts

+
+  if (!Number.isNaN(a)) {
+    return { r, g, b, a };
+  }

  return { r, g, b };


Accordingly, now so.

return { r, g, b, a };

Wondermarin · 2021-07-19T13:52:12Z

src/utils/convert.util.ts

+  if (a !== undefined) {
+    a /= 255;
+    a *= 100;
+  }


if (a) { a /= 255; a *= 100; }

Wondermarin · 2021-07-19T13:52:41Z

src/utils/convert.util.ts

+  if (a !== undefined) {
+    a = Math.round((a / 100) * 255);
+  }


if (a) { a = Math.round((a / 100) * 255); }

Wondermarin · 2021-07-19T13:54:05Z

src/utils/convert.util.ts

-export function rgb2hex({ r, g, b }: Color["rgb"]): Color["hex"] {
-  const hex = [r, g, b].map((v) => v.toString(16).padStart(2, "0")).join("");
+export function rgb2hex({ r, g, b, a }: Color["rgb"]): Color["hex"] {
+  const hex = [r, g, b, a].map((v) => (v !== undefined ? v.toString(16).padStart(2, "0") : "")).join("");


const hex = [r, g, b, a].map((v) => (v ? v.toString(16).padStart(2, "0") : "")).join("");

This change would cause any value equal to zero to emit an empty string, since v ? (...) : (...) implicitly casts to a boolean and zero is a falsy value.

Hawkbat · 2021-07-19T14:07:50Z

Great feedback! I'll implement those changes and update the PR. Making the range 0-1 for alpha makes sense; I'll need to add a bit of extra work in parsing the RGB/HSV text fields since they currently only accept integer values. Something to look out for after the PR is updated.

vercel · 2021-07-19T23:50:32Z

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/wondermarin/react-color-palette/DacboYeFXGUw79B1cWJdXztWMbAH
✅ Preview: https://react-color-palette-git-fork-hawkbat-hawkbat-fc55b7-wondermarin.vercel.app

Hawkbat · 2021-07-19T23:51:53Z

Updated to resolve the feedback, except for things I was unsure about. I tried to reduce some duplicated code as well.

Wondermarin

Except for this, everything else is fine! Thank you for the work, as soon as you update the PR, I will be happy to merge it.

Wondermarin · 2021-07-20T08:53:37Z

src/utils/convert.util.ts


-  return { r, g, b };
+  return { r, g, b, a: Number.isNaN(a) ? undefined : a / 255 };


Since now the alpha channel format is from 0 to 1, you can do this.

const a = parseInt(hex.slice(6, 8), 16) || undefined; if (a) a /= 255; return { r, g, b, a };

ceoshikhar · 2021-07-21T18:16:32Z

src/components/ColorPicker.component.tsx

  dark = false,
 }: ColorPickerProps): JSX.Element => (
  <div className={`rcp ${dark ? "rcp-dark" : "rcp-light"}`} style={{ width }}>
    <Saturation width={width} height={height} color={color} onChange={onChange} />
    <div className="rcp-body">
      <Hue width={width - 40} color={color} onChange={onChange} />
-      <Fields color={color} hideHEX={hideHEX} hideRGB={hideRGB} hideHSV={hideHSV} onChange={onChange} />
+      {alpha ? <Alpha width={width - 40} color={color} onChange={onChange} /> : null}


Is it just me or the && a cleaner way to do optional rendering so that null is not needed.

What I mean is this

{alpha && <Alpha width={width - 40} color={color} onChange={onChange} />}

Is it just me or the && a cleaner way to do optional rendering so that null is not needed.

What I mean is this

{alpha && <Alpha width={width - 40} color={color} onChange={onChange} />}

Oh. I didn't notice. Thank you ❤️

Hawkbat · 2021-07-21T21:52:50Z

Updated! Should address those bits of feedback.

Wondermarin

Great job!

rick-nu · 2021-10-12T18:58:39Z

Switched from react-color to this library, big part of that was because of this feature, thank you @Hawkbat! 💪 and thanks for the clean alternative @Wondermarin 👍

Hawkbat added 2 commits July 17, 2021 16:31

feat: Add alpha prop for alpha channel support.

34673de

Add alpha slider component Add alpha props and alpha slider to components

chore: Update README.md

5306f8a

Hawkbat mentioned this pull request Jul 17, 2021

Alpha channel #10

Closed

Wondermarin added p: medium s: in progress semver: minor t: enhancement labels Jul 18, 2021

Wondermarin reviewed Jul 18, 2021

View reviewed changes

Wondermarin requested changes Jul 19, 2021

View reviewed changes

refactor: Change to use 0-1 range alpha and minor styling changes

8983d36

vercel bot temporarily deployed to Preview July 19, 2021 23:50 Inactive

Hawkbat requested a review from Wondermarin July 19, 2021 23:51

Wondermarin requested changes Jul 20, 2021

View reviewed changes

ceoshikhar reviewed Jul 21, 2021

View reviewed changes

Wondermarin linked an issue Jul 21, 2021 that may be closed by this pull request

Alpha channel #10

Closed

chore: style changes for alpha support code

db718fb

vercel bot temporarily deployed to Preview July 21, 2021 21:52 Inactive

Hawkbat requested a review from Wondermarin July 21, 2021 21:52

Wondermarin approved these changes Jul 22, 2021

View reviewed changes

Wondermarin merged commit aa7c289 into Wondermarin:master Jul 22, 2021

Wondermarin removed the s: in progress label Jul 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add alpha channel support #15

Add alpha channel support #15

Hawkbat commented Jul 17, 2021

Wondermarin left a comment •

edited

Loading

Hawkbat commented Jul 18, 2021

Wondermarin left a comment

Wondermarin Jul 19, 2021

Wondermarin Jul 19, 2021

Wondermarin Jul 19, 2021 •

edited

Loading

Wondermarin Jul 19, 2021 •

edited

Loading

Wondermarin Jul 19, 2021

Hawkbat Jul 19, 2021

Wondermarin Jul 19, 2021

Wondermarin Jul 19, 2021

Wondermarin Jul 19, 2021

Wondermarin Jul 19, 2021

Hawkbat Jul 19, 2021

Hawkbat commented Jul 19, 2021

vercel bot commented Jul 19, 2021 •

edited

Loading

Hawkbat commented Jul 19, 2021

Wondermarin left a comment

Wondermarin Jul 20, 2021 •

edited

Loading

ceoshikhar Jul 21, 2021

Wondermarin Jul 21, 2021

Hawkbat commented Jul 21, 2021

Wondermarin left a comment

rick-nu commented Oct 12, 2021


		return { r, g, b };
		return { r, g, b, a: Number.isNaN(a) ? undefined : a / 255 };

Add alpha channel support #15

Add alpha channel support #15

Conversation

Hawkbat commented Jul 17, 2021

Checklist

Description of Change

Wondermarin left a comment • edited Loading

Choose a reason for hiding this comment

Hawkbat commented Jul 18, 2021

Wondermarin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wondermarin Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Wondermarin Jul 19, 2021 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hawkbat commented Jul 19, 2021

vercel bot commented Jul 19, 2021 • edited Loading

Hawkbat commented Jul 19, 2021

Wondermarin left a comment

Choose a reason for hiding this comment

Wondermarin Jul 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hawkbat commented Jul 21, 2021

Wondermarin left a comment

Choose a reason for hiding this comment

rick-nu commented Oct 12, 2021

Wondermarin left a comment •

edited

Loading

Wondermarin Jul 19, 2021 •

edited

Loading

Wondermarin Jul 19, 2021 •

edited

Loading

vercel bot commented Jul 19, 2021 •

edited

Loading

Wondermarin Jul 20, 2021 •

edited

Loading