components: Add new ColorPicker #33714
Conversation
|
Size Change: -35 kB (-3%) Total Size: 1.03 MB
ℹ️ View Unchanged
|
sarayourfriend
added
[Type] Enhancement
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
sarayourfriend
force-pushed
the
add/new-color-picker
branch
from
1aa690e
to
ac08564
Compare
There was a problem hiding this comment.
Nicely done so far, @sarayourfriend! This was quite a large PR to get through.
I've left a few inline comments, but I also have a few more general pieces of feedback:
- I haven't reviewed any
InputControl-related changes because I assume that they're actually being worked on in a separate PR components: InputControl to TypeScript #33696 , and that this PR will be rebased once those changes land ontrunk - Is there a reason why the conversion to TS of the
SelectControlcomponent isn't also done in a separate PR? - Should we make some small changes to the APIs (props) of the
ColorPickercomponent, in order to match the "legacy"ColorPicker? - I haven't done an exhaustive review in terms of accessibility, but at a quick glance I can see that focus styles are not very prominent (not sure if the original designs have any specs for that) cc @diegohaz
Comparing the Storybook prototype against the latest designs, I could spot a few differences, which I annotated over a couple of screenshots:
| ### `enableAlpha` | ||
|
|
||
| **Type**: `boolean` | ||
|
|
||
| Defaults to `false`. When `true` the color picker will display the alpha channel both in the bottom inputs as well as in the color picker itself. |
There was a problem hiding this comment.
Nit: should we use the same format we've been using for newer components?
| ### `enableAlpha` | |
| **Type**: `boolean` | |
| Defaults to `false`. When `true` the color picker will display the alpha channel both in the bottom inputs as well as in the color picker itself. | |
| ### `enableAlpha`: `boolean` | |
| When `true` the color picker will display the alpha channel both in the bottom inputs as well as in the color picker itself. | |
| - Required: No | |
| - Default: `false` |
|
Yup, definitely aware of the design differences 🙂 However, I'm not sure how best to address them. On one hand, we can change the default styles for these components to match the designs. This would update all of Gutenberg. On the other hand, we could create wrappers for all these components and manually change the styles using
It would also be code we would hopefully anticipate deleting in the near future when the designs of the underlying components were updated. My preference is for the first approach, simply update the existing component designs. However, of course, I don't want to do that all in one PR. So in some sense this PR isn't at all mergable not because it's incomplete but becuase it's uncovered a dearth of dependencies that need to be resolved before it can be merged (esentially all of the visual issues you pointed out save the spacing ones). |
I don't think we should take any cues from the existing ColorPicker component. It's massively over complicated. |
sarayourfriend
force-pushed
the
add/new-color-picker
branch
from
51bfaa8
to
b53bb7e
Compare
If we wanted this I know that this can be done at a later point in time when we promote the g2 |
|
I think my proposal for the ColorPicker is to add a prop like Primarily the issue I have the with the existing ColorPicker is that it's API is far too broad, especially in the change handlers. It also uses "backwards" props like I'd love to fully deprecate the existing ColorPicker entirely, to be honest. Or export the new one under a separate name to allow for good tree-shaking (for example |
Thank you for elaborating. What you suggest is definitely a possibility. This kind of approach should be probably discussed in the context of coming up with a strategy that can work across the whole components library (for example, we may have a similar situation with the Flyout and the Slider component). Definitely not a blocker for this PR |
sarayourfriend
force-pushed
the
add/new-color-picker
branch
from
dc448d4
to
cc5ff35
Compare
|
I think we have to change the output/input value to also be HSL(A) otherwise the HSL encoding is immediately lost the second that it leaves the component and gets passed back in (in the case of a controlled component like in the stories). |
|
I'm not sure how to solve this because changing the external contract to HSL(A) makes it a lot more complicated from a consumer perspective. But on the other hand trying to simplify it and accept any color format as input, allows the user to incorrectly push a hex value which will cause the internals to no longer be able to encode valid HSL(A) values (even if they have no visual difference) |
|
Probably the best tradeoff would be to switch to HSL(A) for both the internal representation and for the accepted format of the We could then expose the |
I'd rather we didn't expose colorize directly... people can easily import tinycolor2 if they want rather than making it part of the public WP api. |
What would you suggest? I'm mirroring the way lots of other of these high-level composition components are structured. I don't think an individual folder and README for each of these internal components makes any sense at all. Do you have a suggestion for this? |
There was a problem hiding this comment.
After the switch to HSL(A)-based input, the UX has definitely improved!
I'm happy to work on unit tests, folder structure, and design refinements in follow-up PRs.
(cc @diegohaz in case you want to give this PR a final look)
What would you suggest? I'm mirroring the way lots of other of these high-level composition components are structured. I don't think an individual folder and README for each of these internal components makes any sense at all. Do you have a suggestion for this?
I agree that one folder for each subcomponent doesn't make sense — especially because these subcomponents are not going to be exported.
My idea was more about splitting the component's "entry point" (e.g. component.tsx) from the remaining sub-components.
Maybe having a few folders grouping sub-components with similar functionality, like pickers/ (with hex-color-picker and rgba-color-picker) and inputs/ (color-input, hex-input, hsl-input, rgb-input).
Another difference compared to "standard" g2 components is that there's no hook.ts file.
As explained before, these considerations should not be considered blockers for this PR.
|
Actually, flagging these accessibility warnings from Storybook
|
The first one seems to be a problem with the react-colorful package. The Do we have control over the elements that react-colorful renders? If so, we should be able to add that prop. |
We don't 😞 It looks like they've already done some accessibility work so I'm sure they'd be open to a PR to fix this: omgovich/react-colorful#57 I'll see about opening one upstream. |
Does it need a hook? It feels like we shouldn't thoughtlessly follow the hook + component pattern if it just adds redirection/complexity to something that can just be a regular component (especially if the hook isn't meant to be reusable, which is the primary benefit of extrapolating things into hooks, but in this case what would be the reusable version of the hook for this?) |
There was a problem hiding this comment.
All accessibility issues that can be fixed on our end are now solved.
I'll see about opening one upstream.
That sounds great, thank you!
Does it need a hook?
You actually make a good point about the logic of this component not needing to be "reusable". I agree with you, it doesn't need a hook.
|
Great work! 🎉 For the record, here's the issue about the lack of the |
|
Hi guys! Thanks for choosing react-colorful for your project! P.S. I noticed that your project using The API is kinda the same as For example, |
| } | ||
| } | ||
| }, | ||
| () => setCopiedColor( colorize( color ).toHex8String() ) |
There was a problem hiding this comment.
I just noticed a minor UX improvement that we could make:
- User clicks on the color, value is copied in the clipboard. The tooltip changes from "Copy" to "Copied!"
- User interacts with other parts of the UI, maybe copies another string of text
- User remembers that they need the color value, and so they hover over the color string again. But the tooltip keeps saying "Copied!", instead of "Copy"
This to say, should we use a timer to unset the copiedColor after some time has passed?
Description
Adds a new vastly simplified ColorPicker implementation based on
react-colorful.How has this been tested?
Storybook.
Screenshots
Gravacao.de.Tela.2021-07-28.as.12.43.58.mov
Types of changes
New feature.
Checklist:
*.native.jsfiles for terms that need renaming or removal).