Update: Custom gradient picker design. #34712
Conversation
jorgefilipecosta
added
[Feature] Design Tools
jorgefilipecosta
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +219 B (0%) Total Size: 1.06 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
update/gradient-picker-design
branch
2 times, most recently
from
72de3db to
5e82c6a
Compare
There was a problem hiding this comment.
Very lovely work. Good progress in the metrics, dimensions and such, and remember to also test duotone:
A few small things. The gradient box should have border-radius: $radius-block-ui;.
The gradient preview itself should have padding left and right, that pushes the left and right-most handles inward. I'm aware that skews the preview a bit, but that's intentional to have that extra clearance for the handle. Hre's a zoom_
In the above, the gradient from yellow to blue starts and ends with clearance. Meaning the part of the gradient as marked by the red squares below, are entirely flat solid colors. Technically it might be sensible to just create extra divs before and after the gradient range, then duplicate the colors as solid backgrounds on those. Make sense?
This transparent swatch, what does it do, do you know?
Higher level, there are a few things that need to fall in place to match this:
There's the collapsible color swatch that sits in an itemgroup, slides the panel right in global styles but opens as a flyout in the post editor, and then the new color picker as a layer under that opening from the gradient handles. That flow is superficially (illustrated here). As just one ingredient on the path, this is a nice one. But while we're here, should we also update some of the context surrounding the gradient picker, such as the the new segmented control for swapping between solid and gradient? Or is that for each consuming panel to address?
| @@ -33,17 +34,17 @@ export const CircleIndicatorWrapper = styled.div` | |||
| `; | |||
|
|
|||
| export const CircleIndicator = styled.div` | |||
| background: ${ COLORS.ui.border }; | |||
| background: #3858e9; | |||
There was a problem hiding this comment.
Can this reference a variable instead?
There was a problem hiding this comment.
Unfortunately no, we don't have a variable for this color.
| @@ -102,3 +97,18 @@ $components-custom-gradient-picker__padding: 6px; // 36px container, 24px handle | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // All these styles should be made generic and changed on the inputs for all components. | |||
There was a problem hiding this comment.
I appreciate the "todo". It's something we might want to look into soon.
There was a problem hiding this comment.
Agreed. We should look into all control/input components and standardize their dimensions and spacing throughout the library of @wordpress/components
| padding-bottom: $grid-unit-10 !important; | ||
| } | ||
| label { | ||
| text-transform: uppercase; |
There was a problem hiding this comment.
The metrics as I've extracted them from the Figma also make this inspector "Section Heading" 11px font-size with a font-weight of 500.
There was a problem hiding this comment.
Hey @jorgefilipecosta , thank you for working on this!
Here are my initial thoughts:
- I'd be curious to know why we're not using
BaseControlanymore. In case it's not satisfying a certain need, we can look into improving the component directly! - Similar question for
NumberControl— why did you decide to useInputControlinstead? - Similarly to what @jasmussen said, I can see a few hardcoded values (especially colors, box-shadows) — it'd be great if we could use values from the shared
utils/folder as much as possible. If there arent't any values in there that could be reused, we should consider adding some of these new values directly to the shared configuration.
Thank you! I'm also asking some of these questions because I'm always interested in knowing if any of the components being used are not offering a certain feature or if they can be improved in general!
| @@ -37,20 +34,18 @@ $components-custom-gradient-picker__padding: 6px; // 36px container, 24px handle | |||
| } | |||
|
|
|||
| .components-custom-gradient-picker__control-point-button { | |||
| border: 2px solid transparent; | |||
| box-shadow: inset 0 0 0 var(--wp-admin-border-width-focus) $white; | |||
| border: 1.5px solid $white; | |||
There was a problem hiding this comment.
Not sure if we should use sub-pixel widths — it can create rendering glitches
There was a problem hiding this comment.
We actually use 1.5px for the focus style across the board, but good catch — that is rendered using box-shadow so as to actually function, whereas I believe borders are rounded to nearest whole.
For additional context, we usually pair the box shadow with a transparent border or outline, because in Windows High Contrast mode (which is looking like it might become a web standard), box shadows are removed entirely, but transparent colors are made opaque. So in those modes, the transparent border would work as the focus style.
There was a problem hiding this comment.
Hi @jasmussen, here we have an issue because we have a 1.5px border and we also need a box-shadow (according to figma). So I'm not sure what we should do it seems hard to have a 1.5 border and also a box-shadow (at least without some complex hack).
There was a problem hiding this comment.
I took the liberty of pushing a fix in 2997a2f, which I don't think is too hacky. It just stacks multiple box shadows by comma separation:
There was a problem hiding this comment.
I wonder if we should be using the CONFIG.borderWidthFocus variable here for the border width — and if we should change its value to be 2px instead of 1.5px (see also related #34598 (comment))
There was a problem hiding this comment.
We should definitely use variables whenever we can. It's worth noting that we use 1.5px for focus styles across the entire editor, so it's a rather big change. What rendering glitches have you seen?
There was a problem hiding this comment.
I noticed this glitch in the Color Picker PR, and changing the value to 2px seemed to fix it for me — although I agree with you about this being quite an impactful change that would need a deeper investigation first.
There was a problem hiding this comment.
But that uses border: https://github.com/WordPress/gutenberg/pull/34598/files#diff-172e1f5e636f1d05b7c28a61e4885cd8fddc9dc390952a5547d271dfb2c7663bR94 which is always rounded to nearest whole pixel. The solution I added in 2997a2f uses box-shadow, which renders on subpixels as well.
There was a problem hiding this comment.
I see, thank you for the explanation! Should we look into applying the box-shadow technique to borders across components (including the ColorPicker PR), then?
There was a problem hiding this comment.
Yes, the benefit of box shadows is that you can stack as many of them as you need, to accomplish very complex designs, such as the 1.5px inner stroke with a subtle outer drop shadow.
The one thing we need to keep in mind, though, is to always pair it with a transparent border or outline, as box-shadows are removed entirely in Windows High Contrast mode, whereas transparent colors are made opaque. By combining both, the shadow is shown in normal modes, and the transparent outline or border is shown in high contrast modes. There's a little more nuance in this video.
jorgefilipecosta
force-pushed
the
update/gradient-picker-design
branch
4 times, most recently
from
be0b70a to
a181c15
Compare
|
Hi @ciampo thank you for your review 👍
I don't think using base control here makes semantic sense. BaseControl is wrapping an input field (NumberControl or InputControl) the input fields already provide the accessibility needed. What advantage do we get with wrapping the field with a BaseControl?
Because the number control has arrows to go up and down on the value and the design does not contain the arrows but contains an angle symbol:
I did some updates and tried to use more variables. I'm still adding a new color that's being used on the angle picker inner-circle #3858e9. It seems we don't have any variable for this color, and it is not clear if the color will be used elsewhere. I guess we can consider using a slightly different color from the one in Figma? cc: @jasmussen. |
|
Hi @jasmussen, I agree we still need considerable work to achieve the final UI. This PR tries to just focus on the custom gradient picker part without touching the surroundings that will be worked on in other PR.
I never saw the swatch before, I think you have some invalid preset gradient and given that it does not render well that swatch appears. |
|
@jasmussen, @ciampo thank you for the reviews. Do you think we are ready to merge or is there something we should fix before? |
I have this: |
|
Took this one for a spin. Massive improvements all around. Here's a direct comparison between the figma metrics and this branch: The discrepancy in the input color is something to look at elsewhere and globally for the project, and the subheading looks right (font weight appears to be mostly a rendering differential). So this is nice. The left and right padding/"clearance" is a bit bigger in the Figma file — can you tweak that easily? From what I can tell, you have about 8px, and the Figma has 16px. A rule of thumb here is that the leftmost handle sits as far from the left side as it sits from the top side of the cell. This "Remove control point" link that appears inside the color picker, I'm assuming that's a separate issue to look at in the Color Picker component, correct?
One last thing: it's fancy that the gradient inside the cell can rotate in addition to it rotating in the canvas. However it means the handles are no longer accurate to the gradient below them: Can we move the direction from the gradient swatch and have it always be left to right? Other than that, looks good to go. |
jorgefilipecosta
force-pushed
the
update/gradient-picker-design
branch
from
a181c15 to
2311d40
Compare
|
Hi @jasmussen, I fixed the bar and now the bar matches the control points independently of the angle. Regarding the remove control point. I think it is something we can address during the color picker replacement. But the Figma file does not provide a mockup of how control points should be removed so as an initial version when replacing color components I guess we can keep the "Remove Control Point" link unless there is another design proposal meanwhile and In that case, we can implement it in a follow-up PR. To be worked on after the color picker is replaced. |
There was a problem hiding this comment.
This is working well, the preview/swatch is looking good, and things are functioning nicely:
This is a good step forward.
I think in future iterations we can unify the handles to be even more like those from the color picker, and actually show the solid color inside the white ring (as opposed to being transparent as they are now). We can also bump the shadow opacity a bit, and enhance focus styles.
The segmented color/gradient control, and the handling of incorrect gradients (I know, bug in my theme.json it seems) feel worth looking at as well.
As already tracked, there are some efforts around unifying the component system as well with regards to input fields, subheadings and such. But this is a good step forward to a component that needs it a fair bit.
So this feels like a good step forward. Thank you!
There was a problem hiding this comment.
I don't think using base control here makes semantic sense. BaseControl is wrapping an input field (NumberControl or InputControl) the input fields already provide the accessibility needed. What advantage do we get with wrapping the field with a BaseControl?
Got it. I'll look a bit more into it and reply if I find anything interesting.
Because the number control has arrows to go up and down on the value and the design does not contain the arrows but contains an angle symbol:
It looks like NumberControl has a hideHTMLArrows property, did you try setting it to false ?
If we need to go with InputControl, hould we set type="number" then on the InputControl ?
Focusing just on the custom picker part it seems the missing part is the flyout color picker and using the new color picker but that part will happen when we replace the pickers.
Yep that's the plan
Regarding the flyout, I guess we will have a component for the flyouts right @ciampo? In that case, I guess we should wait until the component is available.
We already have an experimental Flyout component, but I believe we want to first agree on a good path forward to reconcile it with the legacy Dropdown and Popover components (see #33391 for more context, and feel free to leave a comment there to move the conversation forward).
Do you think we are ready to merge or is there something we should fix before?
Apart from @jasmussen 's latest feedback, there are still a couple of items pending, like this one about a TODO comment
I think in future iterations we can unify the handles to be even more like those from the color picker, and actually show the solid color inside the white ring (as opposed to being transparent as they are now). We can also bump the shadow opacity a bit, and enhance focus styles.
The segmented color/gradient control, and the handling of incorrect gradients (I know, bug in my theme.json it seems) feel worth looking at as well.
As already tracked, there are some efforts around unifying the component system as well with regards to input fields, subheadings and such. But this is a good step forward to a component that needs it a fair bit.
I'm currently working on a tracking issue (#35093) for color-related components in the context of the general UI update that we're going through for the new global styles sidebar.
It'd be great if you could add all existing color gradient issues and the follow-up tasks to this PR!
jorgefilipecosta
force-pushed
the
update/gradient-picker-design
branch
from
2311d40 to
ee7c0cb
Compare
|
Hi @ciampo thank you for the insights I was not aware we had hideHTMLArrows property. I used the NumberControl component with that property. I'm merging this PR and working on an inventory of follow-ups and will leave comment here and on #35093. The follow-ups are mainly generic input field changes, or enhancements/fixes that were already present in trunk without this PR. |
This PR updates the design of the custom gradient picker to the new version.
Part of: #34574