Skip to content
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

Feature: Alpha channel #47

Merged
merged 19 commits into from
Sep 14, 2020
Merged

Feature: Alpha channel #47

merged 19 commits into from
Sep 14, 2020

Conversation

omgovich
Copy link
Owner

@omgovich omgovich commented Sep 11, 2020

ezgif com-video-to-gif

This PR adds RGBA, HSLA, and HSVA color models support.

The internal color model now is HSVA (instead of HSV).

New public components:

RgbaColorPicker
RgbaStringColorPicker

HslaColorPicker
HslaStringColorPicker

HsvaColorPicker

New internal components:

Alpha
AlphaColorPicker

AlphaColorPicker helps me to keep old pickers (without alpha support) lighter.
Only AlphaColorPicker imports Alpha component. Regular ColorPicker doesn't do that, so it's still light.

To avoid code duplication between these two components, I moved all logic to a hook (See hooks/useColorManipulation.ts).

How to test

I added live examples of all picker components to the demo page.
Just run npm run start-demo and scroll down.

Notes

Found out that React.memo(Component) makes a component "un-tree-shakeable".
That's the reason why { HexColorInput } is so big.
So I got rid of some React.memo-s because they affect the budnle's size a lot.

WIP

There is an issue in terms of UI. This edge-state looks a bit weird:
image

Going to discuss that with our designer.

@molefrog @ryanchristian4427 Still WIP, but feedback welcome =)

@omgovich omgovich requested a review from molefrog September 11, 2020 14:56
@omgovich omgovich self-assigned this Sep 11, 2020
@github-actions
Copy link

github-actions bot commented Sep 11, 2020

Size Change: +759 B (21%) 🚨

Total Size: 3.45 kB

Filename Size Change
./dist/index.css 697 B +212 B (30%) 🚨
dist/index.module.js 2.75 kB +547 B (19%) ⚠️

compressed-size-action

@rschristian
Copy link
Contributor

Any reason for not adding Alpha with Hex?

@omgovich
Copy link
Owner Author

omgovich commented Sep 11, 2020

@ryanchristian4427 { HexColorPicker } import size:

master:
1,6 KB

this branch:
1.71 KB

If I include Alpha into ColorPicker under condition like {colorModel.hasAlpha && <Alpha ... />}:
1.93 KB

As I mentioned above, bundlers can't tree-shake it.

@rschristian
Copy link
Contributor

rschristian commented Sep 11, 2020

Sure, but what if we add a separate Hex with Alpha component? Seems a bit odd to add Alpha for all but 1.

HexaColorPicker or something.

@omgovich
Copy link
Owner Author

omgovich commented Sep 11, 2020

We have 11 color pickers now:

// These are based on `ColorPicker.tsx` because their input/output doesn't have alpha-channel (like `rgb(0, 0, 0)`)
// They don't need `Alpha` component (extra 200+ bytes gz)
export { HexColorPicker } from "./components/HexColorPicker";
export { RgbColorPicker } from "./components/RgbColorPicker";
export { RgbStringColorPicker } from "./components/RgbStringColorPicker";
export { HslColorPicker } from "./components/HslColorPicker";
export { HslStringColorPicker } from "./components/HslStringColorPicker";
export { HsvColorPicker } from "./components/HsvColorPicker";

// These are based on `AlphaColorPicker.tsx` which includes `Alpha.tsx` component for output like `rgba(0, 0, 0, 1)`
export { RgbaColorPicker } from "./components/RgbaColorPicker";
export { RgbaStringColorPicker } from "./components/RgbaStringColorPicker";
export { HslaColorPicker } from "./components/HslaColorPicker";
export { HslaStringColorPicker } from "./components/HslaStringColorPicker";
export { HsvaColorPicker } from "./components/HsvaColorPicker";

HEX format doesn't need the alpha channel support.
There is HEX8 format, but I'm not going to support it.

@omgovich
Copy link
Owner Author

omgovich commented Sep 11, 2020

Do you mean that we should support HEX8 format?
It feels like a separate task since it affects HexColorInput as well.

@rschristian
Copy link
Contributor

rschristian commented Sep 11, 2020

The overall amount of components we have doesn't really matter. Even if we had 30 the library is just as tiny at point of use.

But yes, I was talking about 4/8 digit Hex, which is Hex with Alpha. If we're adding Alpha support to all other color pickers we should probably add it to Hex too while we're at it. It would be odd not to.

Edit: As a user, I'd expect Hex w/ alpha if I'm provided HSl w/ alpha, HSV w/ alpha, and RGB w/ alpha. Not having Hex w/ alpha would be confusing and unexpected.

@rschristian
Copy link
Contributor

// These are based on `ColorPicker.tsx` because their input/output doesn't have alpha-channel (like `rgb(0, 0, 0)`)
// They don't need `Alpha` component (extra 200+ bytes gz)
export { HexColorPicker } from "./components/HexColorPicker";
export { RgbColorPicker } from "./components/RgbColorPicker";
export { RgbStringColorPicker } from "./components/RgbStringColorPicker";
export { HslColorPicker } from "./components/HslColorPicker";
export { HslStringColorPicker } from "./components/HslStringColorPicker";
export { HsvColorPicker } from "./components/HsvColorPicker";

// These are based on `AlphaColorPicker.tsx` which includes `Alpha.tsx` component for output like `rgba(0, 0, 0, 1)`
export { RgbaColorPicker } from "./components/RgbaColorPicker";
export { RgbaStringColorPicker } from "./components/RgbaStringColorPicker";
export { HslaColorPicker } from "./components/HslaColorPicker";
export { HslaStringColorPicker } from "./components/HslaStringColorPicker";
export { HsvaColorPicker } from "./components/HsvaColorPicker";

Looking at this reminds me, any reason for there to be no HsvStringColorPicker?

@omgovich
Copy link
Owner Author

omgovich commented Sep 11, 2020

Let's skip HEX8 and HsvString in this PR.
The first one is tricky to add since it might break our primary HEX color model. It's definitely a separate task.
I will HsvString later in a separate PR. Just don't want to make this PR even bigger then it is already =)

I also wanted to add CMYK format support, but postpone it while we have more important stuff to do.

@rschristian
Copy link
Contributor

rschristian commented Sep 11, 2020

Fair.

The HsvString is totally unrelated, only just noticed it. Definitely for another PR.

Hex w/ alpha would be related, but makes sense to separate out if it needs extra work.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@web-padawan web-padawan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two minor comments regarding formats used in default colors.

In case if you don't support IE11, it could be fine to always use rgb and hsl, respectively.
According to caniuse, IE11 is the only browser that does not support alpha in rgb / hsl.

src/components/HslaStringColorPicker.tsx Outdated Show resolved Hide resolved
src/components/RgbaStringColorPicker.tsx Outdated Show resolved Hide resolved
@omgovich omgovich merged commit 609b7ab into master Sep 14, 2020
@omgovich omgovich deleted the feature/alpha branch September 14, 2020 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants