Skip to content

ColorPalette, GradientPicker: fix color picker popover positioning #42989

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
talldan merged 14 commits into from
Aug 12, 2022
Merged

ColorPalette, GradientPicker: fix color picker popover positioning #42989

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f6ce296
ColorPalette: do not ignore popoverProps, always shift, use placement…
ciampo Aug 4, 2022
1e9978b
CustomGradientBar: pass parent ref correctly
ciampo Aug 4, 2022
be50c30
CustomGradientBar: Use placement instead of position
ciampo Aug 4, 2022
72d8b65
Add back placement override for color picker dropdown when in sidebar
ciampo Aug 4, 2022
e49a463
CHANGELOG
ciampo Aug 4, 2022
c0cf5d4
Align popover to bottom center of control point when in sidebar
ciampo Aug 8, 2022
5f2b58e
Remove gradientPickerDomRef prop where not needed
ciampo Aug 8, 2022
b984d33
Update snapshots
ciampo Aug 8, 2022
3371ca5
Update packages/components/src/color-palette/index.js
ciampo Aug 9, 2022
c3fc778
GradientPicker: always render popover below control point, memoize po…
ciampo Aug 9, 2022
e107ec9
Replace custom Button styles by using HStack component
ciampo Aug 9, 2022
ecf677b
Iterate on the popover behavior
ciampo Aug 11, 2022
f731cf2
Adjust ofset when stacking popovers
ciampo Aug 12, 2022
559bb93
Update CHANGELOG
ciampo Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Align popover to bottom center of control point when in sidebar
  • Loading branch information
@ciampo
ciampo committed Aug 11, 2022
commit c0cf5d4a4699a327486bbaef06b7ab68053f90ec
5 changes: 3 additions & 2 deletions packages/components/src/color-palette/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,10 +119,11 @@ export function CustomColorPickerDropdown( {
} ) {
const popoverProps = {
__unstableShift: true,
// Open below the control point (centered aligned) when in the sidebar
...( isRenderedInSidebar
? {
placement: 'bottom-start',
offset: 20,
placement: 'bottom',
offset: 10,
}
: {} ),
...receivedPopoverProps,
Expand Down
6 changes: 2 additions & 4 deletions packages/components/src/custom-gradient-bar/control-points.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,9 @@ function GradientColorPickerDropdown( {
const result = {
className:
'components-custom-gradient-picker__color-picker-popover',
placement: 'top',
};
if ( isRenderedInSidebar ) {
result.anchorRef = gradientPickerDomRef;
result.placement = 'bottom-start';
if ( ! isRenderedInSidebar ) {
result.placement = 'top';
Copy link
Contributor

@talldan talldan Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Screen Shot 2022-08-09 at 12 50 18 pm

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.

Copy link
Contributor Author

@ciampo ciampo Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just pushed 1d30a2a in which:

  • the placement is always set to bottom regardless of the isInSidebar prop (I also tweaked the offset by a couple of px)
  • I've used useMemo also inside CustomColorPickerDropdown (instead of removing it)

Copy link
Contributor Author

@ciampo ciampo Aug 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, after 7c23528, there's currently no need for any popoverProps override (although there will probably be a need for it again soon)

}
return result;
}, [ gradientPickerDomRef, isRenderedInSidebar ] );
Expand Down