Enhance the new color picker design #34598
Conversation
jorgefilipecosta
added
Needs Design
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
I guess #34287 needs to land before I can see this one outside of storybook. This is what I see:
Nice, getting there. A few things:
|
|
|
||
| export const InputWrapper = styled( InputControl )` | ||
| ${ InputControlContainer } { | ||
| padding: 5px 8px; |
There was a problem hiding this comment.
I'm still wrapping my head around .ts files, but when I see bespoke paddings and margins like these it gets me a little worried about code drift. What's the best solution for keeping things the same here? I understand re-used SCSS variables (like the $grid-units we have) isn't possible here. Is it more of a matter of updating the source component used?
There was a problem hiding this comment.
After switching to Emotion, we've been using the space util (packages/components/src/ui/utils/space.ts) in combination with some shared configuration (packages/components/src/utils/config-values.js) — we should probably leverage them here as well, instead of these hardcoded values
(also cc'ing @sarayourfriend in case I missed anything)
|
|
||
| export const NumberControlWrapper = styled( NumberControl )` | ||
| ${ InputControlContainer } { | ||
| width: 96px; |
There was a problem hiding this comment.
Same concern here about bespoke paddings.
On the width, what options do we have to use flex here so the items are automatically spaced and sized by the container?
There was a problem hiding this comment.
The NumberControlWrapper is rendered as a child of <Spacer as={ HStack }>, which means that Spacer is effective a flex container, and that props from HStack can be applied to that instance of <Spacer />. NumberControlWrapper can be treated as a flexbox child.
| @@ -27,7 +26,7 @@ export const InputWithSlider = ( { | |||
| }: InputWithSliderProps ) => { | |||
| return ( | |||
| <Spacer as={ HStack }> | |||
| <NumberControl | |||
| <NumberControlWrapper | |||
| __unstableInputWidth="5em" | |||
There was a problem hiding this comment.
I wonder if we can replace the __unstableInputWidth prop with some CSS directly from styles.ts
|
|
||
| export const NumberControlWrapper = styled( NumberControl )` | ||
| ${ InputControlContainer } { | ||
| width: 96px; |
There was a problem hiding this comment.
The NumberControlWrapper is rendered as a child of <Spacer as={ HStack }>, which means that Spacer is effective a flex container, and that props from HStack can be applied to that instance of <Spacer />. NumberControlWrapper can be treated as a flexbox child.
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
from
4bafa8d
to
f5280f1
Compare
|
Size Change: -27 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
2 times, most recently
from
d847b80
to
65de35a
Compare
|
Hi @jasmussen, @ciampo, Thank you a lot for the reviews, I tried to incorporate your feedback. Mainly by avoiding hardcoded values and reusing shared values and utils (that I was not aware of and really simplify things). There is a missing general change that I tried and failed. It was to change the input height which should be 40 by default and currently is 30. But that general change did not work well and broke the design in other parts already in production: Although it seems we can easily change the height because we have constant for that in practice things don't work parts of the UI stop being centered etc (we should take this opportunity to improve our code and make sure in the future we can easily change the constant). Changing the input field height also probably requires changing things like button height otherwise things don't look nice side by side. So I guess that we should do that task in a different PR focused on it. Where we can properly review it and make sure we don't break any of the existing UI. So, for now, I keep a style specifically for the color picker that sets the inputs to 40 so the color picker already uses the new design and after making the change general we can delete this one-line style. Feel free to have a new look at this PR and let me know what other improvements we can do. Thank you in advance for any additional insights. |
|
It seems like input fields might be the next thing we handle, so we don't have to add too many edgecases like these. 👍 👍 |
There was a problem hiding this comment.
When playing around in Storybook, I noticed how both "thumbs" (the circular dots used to pick a color in the "2d color square" and on the "saturation track") are not perfectly centre-aligned with their white border, and slightly wiggle when the other thumb is being dragged around. These issues are likely caused by sub-pixel rounding/rendering.
color-picker-sub-pixel.mp4
Apart from that, it'd be great to get @mtias and @pablohoneyhoney to take a look at this before merging.
| // All inputs should be the same height so this should be changed at the component level. | ||
| // That involves changing heights of multiple input types probably buttons too etc. | ||
| // So until that is done we are already using the new height on the color picker so it matches the mockups. |
There was a problem hiding this comment.
Could you please open a new issue for this task, and add it to tracking issue #34284?
| @@ -38,7 +38,12 @@ export const HexInput = ( { color, onChange, enableAlpha }: HexInputProps ) => { | |||
| <InputControl | |||
| __unstableInputWidth="8em" | |||
There was a problem hiding this comment.
I know this is not caused by this PR, but I wonder if we could refactor away from the __unstableInputWidth property in this case as well?
(cc @sarayourfriend in case there's a specific reason why we're using this prop)
There was a problem hiding this comment.
I don't have any insight into this prop, sorry!
There was a problem hiding this comment.
That's fine! Thank you for replying anyway :)
There was a problem hiding this comment.
I removed the usage of __unstableInputWidth 👍
|
The sliders look mispositioned, not sure if it's an artefact of storybook alone:
The toggle button is a bit too large:
Most noticeable in toggled state:
The spacing between input and range control is also too tight:
There's something going on in the bottom edge of the picker where it resets values in rgb to 255. Not sure if it's just an issue with how the storybook is setup. Screeny.Video.14.Sep.2021.at.15.32.46.mov |
|
Expanding on the video above, when the reset happens the opacity slider renders like this:
|
karmatosed
added
Needs Design Feedback
Hi @ciampo, Safari: The issue on chrome does not affects just our code even the react colorful samples are affected https://codesandbox.io/s/react-colorful-customization-demo-mq85z?file=/src/styles.css: |
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
2 times, most recently
from
2943d5a
to
be14755
Compare
|
Excellent. Just a few lingering things:
|
| ${ inputHeightStyle } | ||
| `; | ||
|
|
||
| export const DetailsControlButton = styled( Button )` |
There was a problem hiding this comment.
It's a bit unfortunate that this is not a button that can be shown properly with just props. Why do we need to tweak instead of relying on composition of uncustomized Button (is the isSmall prop not enough?)
There was a problem hiding this comment.
The isSmall prop on buttons with just an icon creates a 36-pixel width per 24 pixels height. Here we want a 24x24 pixels button.
There was a problem hiding this comment.
The isSmall prop on buttons with just an icon creates a 36-pixel width per 24 pixels height. Here we want a 24x24 pixels button.
Do you think we should fix it in the button's styles instead?
&.is-small {
height: 24px;
line-height: 22px;
padding: 0 8px;
font-size: 11px;
&.has-icon:not(.has-text) {
padding: 0 8px;
width: 24px;
}
}
According to this style from the button's styles, it's meant to be 24px maybe there's bug?
There was a problem hiding this comment.
Good point, I will try to understand if it is a bug or not that the button appears with 36px, but the padding and other styles suggest it is intentional.
|
A couple more, let's reconcile these two tooltips:
The border radius is correct on the first one, but we should use the darker version not the lighter gray. |
| `; | ||
|
|
||
| export const ColorHexInputControl = styled( InputControl )` | ||
| width: 8em; |
There was a problem hiding this comment.
I guess I have similar questions for all the customized lower level components: InputControl, SelectControl, NumberControl... All of these get specific styles here. While in some cases it's fine, I do wonder whether our components don't compose well enough.
In other words, if we struggle to use the base components ourselves to build higher-level ones, it's most likely that the same struggles will exist for folks trying to use the components in their apps (or other wordpress packages like block-editor, edit-post...)
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
from
ba5eee0
to
c2c78c4
Compare
This is interesting — I experimented with changing the I wonder if we should change |
|
Loving how this is progressing. A few details I'm seeing in Safari. Here’s a shot re: shadows.
And how the handles move far from what was expected. Top is what I see in SB, and bottom what was in the Figma.
There’s a strange jump when clicking on the color handle.
Fields in the Figma had #BBBBBB borders, and the slider had a #CCCCCC What I see in SB.
What was in Figma.
For some reason, the focus states and tooltips aren’t visible for me in Safari.
The desired style for focus would like:
|
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
from
c2c78c4
to
68793d8
Compare
| border: ${ CONFIG.borderWidthFocus } solid rgba( 255, 255, 255, 0 ); | ||
| box-shadow: inset 0px 0px 0px ${ CONFIG.borderWidthFocus } #ffffff; |
There was a problem hiding this comment.
We should probably use the box-shadow technique for borders, as suggested by @jasmussen in #34712 (comment)
There was a problem hiding this comment.
I'm not sure if this technique works for this case given that we need border and box-shadow at the same time.
There was a problem hiding this comment.
You can stack box shadows, that's what I did in 2997a2f.
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
from
68793d8
to
4d196f1
Compare
I made an update to make the circles not as far from the bar. Let me know if there are other improvements we should do.
That was a complex issue. I spend a considerable time debugging the component but it turns out the issue was not in the component. We had two pickers separated by a text displaying the color code in a container that made the three elements equally separated. It turns out moving the cursor changed the color code possibly changing the width of the text and that then repositioned the picker. I fixed the story to avoid these jumps.
Yes, the fields need to have a redesign that affects multiple fields and affects existing components. I think we should have an issue focused on the fields where we change its dimensions borders etc.
Yes, I was able to reproduce the issue in Safari. And the current focus style also does not matches the design. Let's try to have an issue focused on the range control where we address these issues given that the component is already in production. |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com> (+1 squashed commit) Squashed commits: [eeb7fa834e] Implement new color picker design
jorgefilipecosta
force-pushed
the
update/new-color-picker-design
branch
from
4d196f1
to
430b261
Compare
|
Will you follow up on the border to box-shadow? I'm happy to help here. |
|
Merging this PR so we can continue the work, the non addressed issues will be addressed in followups:
If there is any follow-up task that I'm missing just let me know. |
|
@jasmussen any help there would be appreciated :) |
Hi @ciampo, Maybe it depends on the chrome version? |
|
Chrome rounds up subpixels to the nearest whole when applied to |
@jorgefilipecosta I've update tracking issue #34284 with the follow-up tasks you suggested. Let's use that issue to coordinate and update the status of the new color picker work |
Description
Part of #34284.
Applies the design changes specified by @ciampo in order to try to match what was proposed in the mockups:
Screenshots