Fix RGBA calc error when change alpha #184
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.
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.
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:
RgbaStringPickerHslaStringPickerHsvaStringPicker
I'll try to find a better solution.
There was a problem hiding this comment.
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.
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.
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 I was able to fix the type error.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.