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

Jump to bottom

Dropdown: use Popover's new anchor prop #43698

Merged
merged 1 commit into from
Aug 31, 2022
Merged

Dropdown: use Popover's new anchor prop #43698

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/components/src/dropdown/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,10 @@ export default function Dropdown( props ) {
}

const args = { isOpen, onToggle: toggle, onClose: close };
const hasAnchorRef =
const hasPopoverAnchor =
!! popoverProps?.anchor ||
// Note: `anchorRef`, `getAnchorRect` and `anchorRect` are deprecated and
// be removed from `Popover` from WordPress 6.3
!! popoverProps?.anchorRef ||
!! popoverProps?.getAnchorRect ||
!! popoverProps?.anchorRect;
Expand All @@ -110,7 +113,9 @@ 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 }
anchor={
! hasPopoverAnchor ? containerRef.current : undefined
Copy link
Member

Choose a reason for hiding this comment

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

Not an issue with this PR, but I'm not 100% sure what this means in practice 🤔 "If you pass me a popoverProps.anchor*, I'll ignore it but pass my own anchor"? Why not just always pass the containerRef.current? And when we add TS types to this component, should popoverProps.anchor* stuff be omitted?

Copy link
Contributor Author

@ciampo ciampo Aug 31, 2022 •
edited

Choose a reason for hiding this comment

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

The idea is:

"If popoverProps doesn't contain any anchor for the Popover, then I will set my own (containerRef.current)".

Since popoverProps are passed to the Popover a few lines after this comment, whatever anchor is set in them will be passed to Popover

The logic is a bit more complicated because an anchor can be currently passed in multiple different ways. But after we deprecate all other anchor props and we'll be left just with anchor, this logic won't be necessary: we could just always pass anchor, and if popoverProps contains an anchor, it will override the default one.

Makes sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And when we add TS types to this component, should popoverProps.anchor* stuff be omitted?

According to the reasoning from my previous message, it should not be omitted — an anchor passed through popoverProps should override the one set internally by Dropdown as a fallback

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha, I read it the other way around 🙈 The undefined threw me off. Thanks for explaining!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, gotcha, I read it the other way around 🙈 The undefined threw me off. Thanks for explaining!

It's definitely not the most straightforward logic 😅

}
{ ...popoverProps }
className={ classnames(
'components-dropdown__content',
Expand Down