ColorPalette, GradientPicker: fix color picker popover positioning #42989
Conversation
ciampo
requested review from
ntsekouras,
mirka,
talldan,
jasmussen and
chad1008
ciampo
added
[Type] Bug
| placement: 'bottom-start', | ||
| offset: 20, |
There was a problem hiding this comment.
@talldan , do you think that the offset / positioning are working as expected here (when editing gradients in global styles in the site editor) ? In my tests, it felt like the popover is not showing the behavior that I expected
There was a problem hiding this comment.
What are you seeing? Something that tripped me up initially is that Dropdown has a default offset of 13, so 20 is only offsetting by 7 from the default position.
The popover is using the components-custom-gradient-picker__markers-container element as the anchor/reference element, so the positioning looks correct (checked by passing 0 for the offset prop). I'm not sure how it ends up with that as the reference element.
I tried 200 for an offset and it was about 200 pixels below the reference, which is to be expected with the bottom placement—the main axis will be the direction of offset.
There was a problem hiding this comment.
Something that tripped me up initially is that
Dropdownhas a default offset of13, so20is only offsetting by7from the default position.
Woops, that was totally it 🤦 nevermind me!
|
Size Change: +134 B (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
BTW - I notice in WordPress core, the custom color popover is always to the left of the gradient popover (it seems anchored to the popover contents, judging by how it sits just below the top padding, and I think it would be placement Maybe that's the desired behavior? Hopefully Joen can confirm. |
|
Thank you for the welcome! This is a tricky one. Here's a GIF showing the desired test behavior:
The color picker opens downward and to the right from the opening swatch (cropped by the GIF). This is in comparison to trunk, where the picker seems arbitrarily positioned:
The desired behavior in the past was for each popover to open in a stacking behavior leftwards from the ItemGroup button — swatch popover first, then the color popover left of that. A bit like this inspector mockup where I hacked the CSS:
My opinion: that hasn't been entirely successful.
An older mockup, is to enable the swatch panel to slide rightwards to the color picker:
It solves the popover in popover problem, but it's still not clear that would be a better user experience, moreover, it's likely going to be a lot of work. So outside of a larger effort, we can either land this PR and drop the stacking behavior, or see if we can restore it to its original behavior (the inspector mockup above). Question: is there a substantial code quality improvement in dropping the stacking behavior? I imagine there might be, and if we do go that way, I'd prefer prefer the popover open down and center, rather than down and right. Like so:
I'm keen on other opinions. |
ciampo
force-pushed
the
fix/gradient-picker-control-point-popover-positioning
branch
from
a064356
to
7d87cf5
Compare
| @@ -357,6 +357,11 @@ exports[`ColorPalette Dropdown should render it correctly 1`] = ` | |||
|
|
|||
| <Dropdown | |||
| contentClassName="components-color-palette__custom-color-dropdown-content" | |||
| popoverProps={ | |||
There was a problem hiding this comment.
Just noting that this snapshot change is due to a change in CustomColorPickerDropdown (in packages/components/src/color-palette/index.js), where previously the __unstableShift prop was set only when isRenderedInSidebar was true — while, with the changes in this PR, __unstableShift is always set to true
|
Hey @jasmussen , I made some changes, and the popover should now open below (and centred) under each control point in the gradient bar, if I understood this piece of feedback correctly:
Otherwise, I'm also happy to try and replicate what you called the "original behavior":
With respect to a deeper refactor, I personally never liked having "nested" popovers, and so I'd be in favour of having something similar to the older mockup that you shared, where the swatch panel slides rightwards to the color picker (although, as you guessed, that would definitely be more than a simple refactor). |
| result.anchorRef = gradientPickerDomRef.current; | ||
| result.position = 'bottom left'; | ||
| if ( ! isRenderedInSidebar ) { | ||
| result.placement = 'top'; |
There was a problem hiding this comment.
I realise it's a bit like this in trunk, but this line makes the duotone color picker look like this, overlapping the block toolbar:
It might be good to simplify it and have GradientColorPickerDropdown always positioned underneath the control point. When there's not enough space Popover will already flip it to the top.
Would be good to get more thoughts on this from @jasmussen and possibly @ajlende.
Another note is that the useMemo here could be removed, as CustomColorPickerDropdown doesn't respect it and creates a new object on every render.
There was a problem hiding this comment.
Just pushed 1d30a2a in which:
- the
placementis always set tobottomregardless of theisInSidebarprop (I also tweaked theoffsetby a couple of px) - I've used
useMemoalso insideCustomColorPickerDropdown(instead of removing it)
There was a problem hiding this comment.
Actually, after 7c23528, there's currently no need for any popoverProps override (although there will probably be a need for it again soon)
| @@ -112,19 +112,27 @@ function MultiplePalettes( { | |||
| ); | |||
| } | |||
|
|
|||
| export function CustomColorPickerDropdown( { isRenderedInSidebar, ...props } ) { | |||
| export function CustomColorPickerDropdown( { | |||
| isRenderedInSidebar, | |||
There was a problem hiding this comment.
Not introduced here, but this is the first time I've seen it. isRenderedInSidebar doesn't seem like it should be in the components package. The prop is very specific to the editor's right-hand sidebar. What if there's a sidebar on the left of the screen?
It looks like it was a late fix for a WordPress release - #37115. @jorgefilipecosta, as you introduced this would you be willing to revisit it?
My feeling is that it should be more generic, like a childPopoverPlacement prop that's passed through to any popovers that should cascade in a given direction.
There was a problem hiding this comment.
I agree 100% on this one, and coincidentally I have just opened an issue with a few changes that I'd like to see for GradientPicker:
My feeling is that it should be more generic, like a
childPopoverPlacementprop that's passed through to any popovers that should cascade in a given direction.
For the specific prop, it will largely depend on what we decide design-wise — let's see how this conversation evolves — but in general, I'd love to get rid of that isRenderedInSidebar prop (also considering how many times it's forwarded through the various component chain!)
There was a problem hiding this comment.
I agree it would be good to get rid of isRenderedInSidebar it was added as a short fix for a visual issue.
There was a problem hiding this comment.
@jorgefilipecosta , would you have any capacity to help with the work on this component, also since you are its main author ?
There was a problem hiding this comment.
Hi @ciampo, I will try to find some availability to work on some enhancements to this component.
There was a problem hiding this comment.
That's great, thank you! I've assigned the issue with the list of enhancements — feel free to ping for any help and for reviews :)
|
Thank you for the fast and impressive work, and thank you Dan for the very helpful pointers towards the changes beyond just gradients. For the gradients specifically, they do feel more contextual with the popover: I failed to think about the other places where this component is used, and per Dan's pointers, they aren't optimal:
(Yes Duotone should probably always open control points downwards)
Specifically this one, clicking the main color swatch to customize it, feels confusing. That does make me feel like we should seek back to the original behavior. Sorry about the detour. On a separate track with less urgency, yes, we should still think about how we can improve this flow overall. Whether that's the sliding interface or something else — this PR has helped put into clarity what needs to be considered here. Thanks again. |
|
Alright, I've applied some of the suggested changes: in particular, with the latest changes in this PR all dropdowns opening within a This is the thing: with the new desired behavior, the
This also links well with @talldan 's comment above. From what I gathered, in order to achieve the new desired behavior we'd need to:
This is definitely a much larger job than the current size of the PR, and therefore I'll wait for everyone's feedback on the proposed solution before working on it. It would be great if we could avoid the many layers of prop drilling the currently are needed to forward the |
|
Just to take stock of the current status. Here's what trunk looks like: Here's what this branch looks like:
As discussed on the thread, there's a lot to fix in terms of popover appearances and positions, though I did note a slight preference for opening like trunk does at least for colors. However as is also clear, the bug to be fixed is the one for color stops in the gradient. In order to fix that, is it fine that we open down/center from the opening color stops across the board? Probably, yes. Especially so we can land a fix for 6.1. I'd love a quick input from @javierarce or @jameskoster if they have alternative ideas, though. One thing we'd need to fix if we go with this approach, though, is that the animation indicates a top left origin, and it should indicate a top center origin. It's hard to see with the fast animation, but if you look for it, it's there. Nice work. |
|
I've always found it a bit confusing that we combine presets and custom values in the same panel. We could eliminate the double popovers altogether if we did something like:
I would probably combine preset solids and gradients too, but that's another story. Anyways...
The overlapping popovers feels a bit awkward for solid colors, but not necessarily a regression. It feels like a fair trade-off to fix the original bug. |
|
I will also try to summarize the current situation, in case we can get to a simpler solution for the purpose of this PR (which originally aimed at being a "quick" bug fix):
Currently, the behaviour of the color picker popover in
|
I think there's the possibility of three configurations:
|
Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
ciampo
force-pushed
the
fix/gradient-picker-control-point-popover-positioning
branch
from
7c23528
to
767e126
Compare
|
@talldan's proposed solution makes sense to me — I went ahead and tried to implement it with my latest changes. Here's what the "color picker in a popover" behaves like across the editor: Kapture.2022-08-11.at.19.24.40.mp4What do folks think? |
ciampo
force-pushed
the
fix/gradient-picker-control-point-popover-positioning
branch
from
767e126
to
ecf677b
Compare
|
Thanks for your effort. If the code isn't too painful, I think that's an excellent path forward, even if we know we want to revisit this in the future. If you can, I'd love to put 12px of space between the color picker and the swatch palette, though — just like the space between the ItemGroup item and the swatch palette. And the color picker still opens from top left, as opposed to top center, for the gradient stops. Nice work! |
|
Hey @jasmussen , I've updated the offset between the color picker and the swatch palette:
I will work on improving the popover opening animations in a follow-up PR. I believe this PR is ready for a final review and hopefully we can go ahead and merge it! |
|
Screenshot looks good to me (maybe give or take some border math, is it slightly larger than the space between palette and itemgroup?) but nothing to block it. I tried compiling the branch but for whatever reason I'm still seeing the old spacing. Probably something on my side. But in any case, screenshot looks good! |
It could be — nothing that we can't tweak in a follow-up if necessary (in case you wanted to test it on your machine, these would be the lines of code responsible for positioning that popover) Do you think this PR is good enough to get an approval and merge? |
ciampo
changed the title
talldan
deleted the
fix/gradient-picker-control-point-popover-positioning
branch
What?
This PR aims at improving the positioning of the color picker's popover in all color-related widgets (mostly
ColorPalette, andCustomGradientBarfor gradients) after #40740 introduced a regression that stopped the forwarding ofpopoverPropscorrectly in theColorPalettecomponent (see change).This PR also slightly tweaks the positioning of the popover for all cases.
Why?
With this change, the
GradientPickercomponent can overridepopoverPropsagain as it was previously intended.How?
popoverPropsin theCustomColorPickerDropdowncomponentbottomof the gradient's control point (centred on the x axis)bottomof its anchor (centred on the x axis), like for the gradient control points.shift, meaning it can't render outside of the viewportplacementproperty, instead of the legacypositionHStackcomponent instead of custom CSS styles to minimize the amount of style overridesTesting Instructions
In Storybook, verify that the popover is always rendered inside the viewport (i.e. shifting behavior is enabled)
In the site editor, try to edit the available gradients palette and focus on the popover positioning for each color stops
Screenshots or screencast
Note: the following screenshots / videos may be out of date. Please refer to the latest comments in this PR for the final popover behavior
GradientPickerin Storybook,trunk:gradient-bar-trunk.mp4
GradientPickerin Storybook, this PR:gradient-bar-pr.mp4
Popover for color stops in gradient picker, site editor,
trunk:custom-gradient-trunk.mp4
Popover for color stops in gradient picker, site editor, this PR:
custom-gradient-pr.mp4
(@jasmussen , welcome back! I may need some guidance here about what is the expected popover positioning when editing color stops in the site editor — see video above)