feat(@clayui/color-picker): Convert to use 8 digit hex colors and add alpha slider #5174
Conversation
There was a problem hiding this comment.
I think if we want the alpha to continuously update with onChange, we need to restructure the components
Yeah, I'm not sure why this is happening, I hope I can delve deeper into it to understand the reasons but the last time I researched this is that using the native range with React doesn't work very well and there are several libraries implementing the slider from scratch probably because of this issue.
It could also be that calling onChange here can cause a lot of rendering and lose reference to the element in the DOM which causes the mouse to be lost every time when interact with the slider.
I was thinking it might be a good idea to add a setting to color picker to disable 8 digit hex support.
I think instead we can make it smarter, because in this implementation we are always converting to hex with 8 digits, I think we should just convert to hex8 when the transparency is less than 1 instead.
I added some initial comments, but I think we can improve a few more things, I'll clone your pr locally and do some more tests to understand some behaviors.
| style: { | ||
| color: string; | ||
| }; |
There was a problem hiding this comment.
In that case since you just want color I would say just go with that property instead of having a property that only has the style, this is easier to use the component even though it is internal. We don't always need to follow the native attribute pattern to create React components, which makes things simpler, we can simplify some things.
| /** | ||
| * Callback function for when the hue value changes | ||
| */ | ||
| onChange: (event: any) => void; |
There was a problem hiding this comment.
The type for React event callback in this case can be: (event: React.KeyboardEvent<HTMLInputElement>) => void
| const ClayColorPickerAlpha = ({ | ||
| style, | ||
| value = 0, | ||
| onChange = () => {}, |
There was a problem hiding this comment.
In those cases where onChange is required it would not be necessary to add a noop function, although we add a lot in some places in Clay at the beginning I really don't like the idea because it makes it difficult to debug bugs when you have a noop function as a default property.
- Styles the clay-range element and resets default values output by the clay-range-input-variant so base styles aren't duplicated by variants
…thumb for range with progress
- This has better support for older browsers
- Use the browser default thumb for clay-range-progress-none
- Adds new hue range input using native input range
- Adds alpha input using native input range
- Increase max height of Clay Color Dropdown Menu
- move alpha transparency background-image to the input
|
@matuzalemsteles I made the changes. Alpha less than 1 will generate hex8 and hex6 for others and the tests should be passing. |
|
Hey @pat270 I'm taking a look at it now, i'm seeing how we can better handle the states and be able to use |
|
@pat270 I tried to make some more changes to try to decrease the number of renders but it seems that React is not working well with the native range, maybe we have to offer a solution as before and add accessibility using ARIA. Even if we keep this behavior as it is now of just updating at the end, it seems like a bad experience for the user to not be able to see the color change in real time. I've tried several things to get realtime color change using the native slider but it's still pretty bad because it keeps losing the mouse drag all the time when dragging the slider. I think we'll have to do the same for #5156 |
|
Putting this on hold, will revisit in the future. |
fixes #5166 issue #5132
I couldn't get the range input to work nicely with color picker. The alpha value updates when the mouse / key is released. I think if we want the alpha to continuously update with
onChange, we need to restructure the components. I'm hoping there is a better way though. I included the CSS updates in #5156 in this pr.I was thinking it might be a good idea to add a setting to color picker to disable 8 digit hex support.