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.

Sign up for GitHub

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

Fix RGBA calc error when change alpha #184

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Fix RGBA calc error when change alpha #184

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
701eae2
Fix RGBA calc error when change alpha
t-hamano Jul 18, 2022
ce3b546
Add updateAlpha prop to colorModel
t-hamano Jul 29, 2022
ded8fd9
Merge branch 'master' into fix/rgba-calc-error
t-hamano Aug 16, 2022
31cde59
Fix type error
t-hamano Aug 16, 2022
f68766e
Refactoring with Generics
t-hamano Aug 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/hooks/useColorManipulation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,18 @@ export function useColorManipulation<T extends AnyColor>(
// Trigger `onChange` callback only if an updated color is different from cached one;
// save the new color to the ref to prevent unnecessary updates
useEffect(() => {
let newColor;
const { a, ...hsv } = hsva;
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const { a: _, ...cacheHsv } = cache.current.hsva;

// When alpha channel is changed, use cached RGB values to prevent rounding errors
const newColor = equalColorObjects(hsv, cacheHsv)
? Object.assign({}, cache.current.color, { a })
Copy link
Owner

Choose a reason for hiding this comment

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

Hi! Thanks for the contribution!

We should keep in mind that cache.current.color can be a string, so this code fixes only object-based color models and (as far as I see) breaks string one (like rgba(200, 200, 200, 0.5)). But I think we're somewhere close)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply!
As you mentioned, I confirmed that on the local demo page, an error occurs when the alpha value changed in the following component:

  • RgbaStringPicker
  • HslaStringPicker
  • HsvaStringPicker

I'll try to find a better solution.

Copy link
Author

Choose a reason for hiding this comment

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

One idea is to add a function to the properties of colorModel that updates only the alpha value:

// HsvaStringColorPicker
const colorModel: ColorModel<string> = {
  defaultColor: "hsva(0, 0%, 0%, 1)",
  toHsva: hsvaStringToHsva,
  fromHsva: hsvaToHsvaString,
  equal: equalColorString,
  // Detects alpha values from strings and replaces
  updateAlpha: updateAlphaFromString,
};

// HsvStringColorPicker
const colorModel: ColorModel<string> = {
  defaultColor: "hsv(0, 0%, 0%)",
  toHsva: hsvStringToHsva,
  fromHsva: hsvaToHsvString,
  equal: equalColorString,
  // Do nothing
  updateAlpha: (value) => value,
};

Then...

const newColor = equalColorObjects(hsv, cacheHsv)
  // Regardless of the type of `cache.current.color`, only the alpha value should be properly updated
  ? colorModel.updateColorAlpha( cache.current.color, a )
  : colorModel.fromHsva(hsva);

Copy link
Owner

@omgovich omgovich Jul 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

You know what? It's actually a good idea!
If we make it optional (not required in every color model), it won't damage a total bundle size dramatically.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, then I will try using this approach 👍
This property should only be added to components that support alpha:

const isSupportAlpha = 'updateAlpha' in colorModel;
const newColor = equalColorObjects(hsv, cacheHsv)
  ? isSupportAlpha ? colorModel.updateColorAlpha( cache.current.color, a ) : cache.current.color
  : colorModel.fromHsva(hsva);

: colorModel.fromHsva(hsva);

if (
!equalColorObjects(hsva, cache.current.hsva) &&
!colorModel.equal((newColor = colorModel.fromHsva(hsva)), cache.current.color)
!colorModel.equal(newColor, cache.current.color)
) {
cache.current = { hsva, color: newColor };
onChangeCallback(newColor);
Expand Down