Fix some issues on the color picker component; Remove tinycolor2; #35562
Conversation
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: -5.26 kB (0%) Total Size: 1.07 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
2 times, most recently
from
c845a39 to
d1bc697
Compare
There was a problem hiding this comment.
Thank you @jorgefilipecosta for working on this! The new ColorPicker component is getting more polished every week!
I think we should follow the readme and make the component receive a string and pass a string to onChange. This way it becomes consistent with the other components like the color palette and custom gradient picker.
During these changes, we also remove the tinycolor2 package.
This is going to be the main point of conversation for this PR. I seem to remember that using the HSL | HSLA format for interfacing with the component was a specific choice — I'm going to cc @sarayourfriend who should have more context about this.
on the readme we said it received a string color property representing the color
If what I just said above holds true, then this must have been overlooked in previous reviews. Thank you for spotting it!
Finally, we should add a changelog entry (the contents of which are still TBD based on the outcome of the previous conversation)
jorgefilipecosta
changed the title
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
from
d1bc697 to
5dfc073
Compare
|
Hi @ciampo thank you for reviewing this PR.
The color palette component that uses the color picker inside (but also includes the presets) has an API where value is string and onChange passes a string. That API is public for a long time and there was never feedback to change it. I think this component should just replicate that. |
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
from
2f12e9e to
1623355
Compare
|
Hi @ciampo, your feedback was applied let me know if there are other changes we should do. |
Thank you for letting me know. I'm going to take a look at this soon — in the future, could you add the code changes to new commits, instead of amending and force pushing the same commit? It would make your changes much more explicit to identify and the review process easier |
ciampo
added
[Feature] UI Components
There was a problem hiding this comment.
Thank you for giving a more detailed explanation about the change to use a string format instead of HSL | HSLA — I actually agree that it could be a good solution here, but I was slightly confused by the mismatch between the internal implementation and the README docs. We will also need to make sure that the issue that caused the switch to the HSL format can't be replicated with this refactor either.
I had a deeper look and left a few more comments. I also checked the component in Storybook and I couldn't find any issues with it!
Having this component written in TypeScript definitely made this review much easier — it's a pity that we can't (yet) type the hex and hex8 types more strictly than a string, but hopefully we'll get there!
|
|
||
| const isTransparent = rawHex === '000000' && rgb.a === 0; | ||
|
|
||
| const hex = isTransparent ? 'transparent' : `#${ rawHex }`; |
There was a problem hiding this comment.
Since this is a legacy adapter, we should not delete this logic (even if I agree that it doesn't make much sense) (see the legacy component's implementation)
There was a problem hiding this comment.
Hi @ciampo, I don't think we must keep this logic. It was never documented that for a transparent color we need to pass the string transparent as the hex value. In fact, I think passing transparent as the hex value goes against the expectation that a hex color is passed ( we are passing a named color instead) so I guess we can apply this change and remove this logic from our codebase.
There was a problem hiding this comment.
I absolutely agree that this code doesn't make much sense — I'm just trying to be very careful about introducing unnecessary breaking changes.
Having said that, since colordColor.toHex() will output a hex8 string including the alpha channel when needed, I feel quite confident that this won't cause any issues (especially when used as a CSS value).
But let's keep an eye on potential breakages around the transparent keyword and let's be ready to revert the removal of this logic
jorgefilipecosta
added
the
Needs Dev Note
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
2 times, most recently
from
bc36e4e to
45e71e7
Compare
|
Hi @ciampo thank you for another pass on this PR most of the suggestions were incorporated. I think this PR should be ready to merge. |
There was a problem hiding this comment.
Thank you for working on this, @jorgefilipecosta !
I think even with HSL we still have the issue of flunky UI on the HSL picker because colors ended up serialized as strings if not inside the component at the block attributes level
I'm not sure I follow entirely, but I think that, at this point, we should keep your current solution (i.e using hex strings) since it uses the same conventions as the previous ColorPicker and as the other color-related components.
We can always make changes at a later stage if we think that the component's UX needs improving
so the way to solve the issue seems to be a follow-up PR that adds some local state in the HSL picker keeping the API of the component the same receives strings and passes strings.
That's an interesting approach that we should definitely investigate. The ideal solution would keep the public APIs unchanged, and would avoid exposing internal dependencies (e.g. colord).
Finally, I noticed that this conversation was marked as solved without being addressed — would you mind taking a quick look before merging?
|
|
||
| const isTransparent = rawHex === '000000' && rgb.a === 0; | ||
|
|
||
| const hex = isTransparent ? 'transparent' : `#${ rawHex }`; |
There was a problem hiding this comment.
I absolutely agree that this code doesn't make much sense — I'm just trying to be very careful about introducing unnecessary breaking changes.
Having said that, since colordColor.toHex() will output a hex8 string including the alpha channel when needed, I feel quite confident that this won't cause any issues (especially when used as a CSS value).
But let's keep an eye on potential breakages around the transparent keyword and let's be ready to revert the removal of this logic
jorgefilipecosta
force-pushed
the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
from
45e71e7 to
8706c2d
Compare
jorgefilipecosta
deleted the
update/fix-issue-on-color-picker-component-remove-tiny-color
branch
Closes #34286
We had some issues with the API of the color picker component on the readme we said it received a string color property representing the color but in fact, it received a hsla color object, the onChange was documented as receiving a string when in fact it also received a hsla color object.
I think we should follow the readme and make the component receive a string and pass a string to onChange. This way it becomes consistent with the other components like the color palette and custom gradient picker.
During these changes, we also remove the tinycolor2 package.
How has this been tested?
I verified the color picker still works as expected on the color palette and the gradient picker.