Skip to content
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

Popover, CustomGradientPicker, Dropdown: Fix positioning of popover when used in a dropdown #41361

Merged
merged 2 commits into from
May 27, 2022
Merged

Popover, CustomGradientPicker, Dropdown: Fix positioning of popover when used in a dropdown #41361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e49ac7
Popover, CustomGradientPicker, Dropdown: Fix positioning of popover w…
andrewserong May 26, 2022
076a7dd
Add changelog entry
andrewserong May 26, 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
4 changes: 4 additions & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

## Unreleased

### Bug Fix

- `Popover`, `Dropdown`, `CustomGradientPicker`: Fix dropdown positioning by always targeting the rendered toggle, and switch off width in the Popover size middleware to stop reducing the width of the popover. ([#41361](https://github.com/WordPress/gutenberg/pull/41361))

### Enhancements

- `SelectControl`: Add `__nextHasNoMarginBottom` prop for opting into the new margin-free styles ([#41269](https://github.com/WordPress/gutenberg/pull/41269)).
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/custom-gradient-picker/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export function getGradientAstWithControlPoints(
return {
length: {
type: '%',
value: position.toString(),
value: position?.toString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was throwing errors in Storybook due to position being undefined. To prevent those errors, I've added in optional chaining, which seemed to do the trick. It doesn't appear to change anything in real world usage in the editor, but made it easier to log out values while debugging in storybook.

Copy link
Member

Choose a reason for hiding this comment

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

When will it error in storybook? Are there reproduction steps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was erroring when the component is first loaded if you navigate to ?path=/story/components-customgradientpicker--default and open up the console. I think in the story for this component, when it's initially mounted it doesn't yet have any position settings which caused the issue. We should probably also update the story, but making this function a little more tolerant to empty values was a quick win 🙂

},
type: a < 1 ? 'rgba' : 'rgb',
value: a < 1 ? [ r, g, b, a ] : [ r, g, b ],
Expand Down
6 changes: 5 additions & 1 deletion packages/components/src/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,11 @@ export default function Dropdown( props ) {
// This value is used to ensure that the dropdowns
// align with the editor header by default.
offset={ 13 }
anchorRef={ ! hasAnchorRef ? containerRef : undefined }
anchorRef={
! hasAnchorRef
? containerRef?.current?.firstChild // Anchor to the rendered toggle.
: undefined
}
Comment on lines +111 to +115
Copy link
Member

Choose a reason for hiding this comment

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

This always feels weird to me that the prop is called anchorRef but it can also accept DOM elements 😆.

{ ...popoverProps }
className={ classnames(
'components-dropdown__content',
Expand Down
7 changes: 4 additions & 3 deletions packages/components/src/popover/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,11 @@ const Popover = (
__unstableForcePosition
? undefined
: size( {
apply( { width, height } ) {
apply( sizeProps ) {
const { height } = sizeProps;
if ( ! refs.floating.current ) return;

// Reduce the height of the popover to the available space.
Object.assign( refs.floating.current.firstChild.style, {
maxWidth: `${ width }px`,
maxHeight: `${ height }px`,
overflow: 'auto',
} );
Expand All @@ -194,6 +194,7 @@ const Popover = (
shift( {
crossAxis: true,
limiter: limitShift(),
padding: 1, // Necessary to avoid flickering at the edge of the viewport.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what it is about the particular settings we're using, but I couldn't get everything to work correctly (removed maxWidth on the size middleware, and a popover at the edge of the screen) without adding at least 1px of padding to the shift middleware. Otherwise We'd see jiggling like in this comment: #41268 (comment)

I'm hoping that adding 1px padding here (which only gets applied at the edge of the viewport) will be viable in the shorter-term. Though it'd be good to get to the bottom of why the issue occurs without the padding.

} ),
hasArrow ? arrow( { element: arrowRef } ) : undefined,
].filter( ( m ) => !! m );
Expand Down