-
-
Notifications
You must be signed in to change notification settings - Fork 106
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
Fix RGBA calc error when change alpha #184
base: master
Are you sure you want to change the base?
Conversation
Sorry I didn't check carefully enough, but I noticed that package size limit has exceeded by few bytes. |
src/hooks/useColorManipulation.ts
Outdated
|
||
// When alpha channel is changed, use cached RGB values to prevent rounding errors | ||
const newColor = equalColorObjects(hsv, cacheHsv) | ||
? Object.assign({}, cache.current.color, { a }) |
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.
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)
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.
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.
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.
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);
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.
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.
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.
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);
I have changed it so that it can update the alpha value for any type of I think the components are working correctly but I couldn't solve the type error 😅 |
✅ I was able to fix the type error. Hope this PR helps! |
I'd like to add my vote to getting this merged if possible. I'm running into an issue where the color starts out as 239,239,239 and then is rounded to 240,240,240 when changing the alpha slider. I'm hoping this will fix the issue. |
Fix: #163
I believe this error is due to an unavoidable rounding error in the process of inter-converting RGB and HSV with integer values.
However, I thought this error would be avoided by retaining the cached RGB values when the alpha value is changed.
I don't know if this change is sufficient, but it appears to have worked correctly for me.
I hope this will be helpful to fix the problem.