-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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.
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
Dropdown: use Popover's new anchor prop #43698
Dropdown: use Popover's new anchor prop #43698
Conversation
Size Change: +5 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
d3360fd
to
d12a6a6
Compare
76ee67c
to
fb73356
Compare
d12a6a6
to
78dbb98
Compare
78dbb98
to
86ce7c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm kind of confused by the underlying logic, but the PR itself looks good!
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 😅
* Popover: add new anchor prop, mark other anchor props as deprecated * Add `anchor` prop to Storybook * Add WP version for deprecated props removal * Do not render fallback anchor if there is already a prop-derived anchor * Block inbetween inserter: use Popover's new anchor prop (#43693) * BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps * ListViewDropIndicator: use Popover s new anchor prop (#43694) * Temporarily disable derpecation warnings * Block toolbar: use Popover's new anchor prop (#43692) * Block toolbar: use anchor prop instead of anchorRef.{top,bottom} * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Lena Morita <lena@jaguchi.com> * Dropdown: use Popover s new anchor prop (#43698) * BlockPopover: prevent error when `selectedElement` is not defined * Try to avoid infinite loop * Update PanelRow docs * Edit navigation menu actions: use Popover s new anchor prop * BorderBoxControl: use Popover's new anchor prop (#43789) * BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null` * Image URL Input: use new anchor prop for Popover (#43784) * Image URL Input: use new anchor prop for Popover * Prevent value from being `null` * Edit site Actions: use new anchor prop for Popover (#43810) * Buttons block: use new Popover anchor prop (#43785) * Buttons block: use new `anchor` prop for `Popover` * Prevent anchor value from being `null` * Navigation block: use new anchor prop for Popover (#43786) * Navigation block: use new `anchor` prop for `Popover` * Use anchor for the Navigation submenu block too * Prevent anchor value from being `null` * Post Date block: use new anchor prop for Popover (#43787) * Post Date block: use new `anchor` prop for `Popover` * Prevent anchor value from being `null` * Tooltip: refactor using Popover's new anchor prop (#43799) * Tooltip: use Popover s new anchor prop * Use internal state to force re-renders when the anchor ref changes * Simplify code * Improve docs around using state instead of refs for the anchor element * Allow `anchor` to be `null` * Edit Post: use Popover's new anchor prop (#43808) * Edit Post: use Popover s new anchor prop * Update comment * SImplify code * Update packages/edit-post/src/components/sidebar/post-schedule/index.js Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Allow passing a `null` anchor Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Refactor `useAnchorRef` and related components to work with the new Popover `anchor` prop (#43713) * useAnchorRef: return a VirtualElement instead of a range * Update useAnchorRef usage in FormatToolbarContainer, use anchor prop * Update remaining `useAnchorRef` usages, switch to the `anchor` prop * useAnchorRef: normalize `null` returns to `undefined` as it is not a valid `anchor` value * Revert changes to native RichText component * Update docs * Allow useAnchorRef to return `null` * Re-enable deprecation warnings * Remove fall back to `undefined` from `null` * Ensure reactive updates when the popover anchor updates * Refactor SocialLinkEdit component to use `anchor` instead of `anchorRef` * CHANGELOG * Add new `useAnchor` hook instead of changing existing `useAnchorRef` hook * Fix API docs * Update Popover unit tests * Remove unused import * Use DOMRect in the DomRectWithOwnerDocument type * Improve the wording of deprecation warnings * Put more emphasis on storing anchor in local state Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
* Popover: add new anchor prop, mark other anchor props as deprecated * Add `anchor` prop to Storybook * Add WP version for deprecated props removal * Do not render fallback anchor if there is already a prop-derived anchor * Block inbetween inserter: use Popover's new anchor prop (#43693) * BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps * ListViewDropIndicator: use Popover s new anchor prop (#43694) * Temporarily disable derpecation warnings * Block toolbar: use Popover's new anchor prop (#43692) * Block toolbar: use anchor prop instead of anchorRef.{top,bottom} * Update packages/block-editor/src/components/block-popover/index.js Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Lena Morita <lena@jaguchi.com> * Dropdown: use Popover s new anchor prop (#43698) * BlockPopover: prevent error when `selectedElement` is not defined * Try to avoid infinite loop * Update PanelRow docs * Edit navigation menu actions: use Popover s new anchor prop * BorderBoxControl: use Popover's new anchor prop (#43789) * BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null` * Image URL Input: use new anchor prop for Popover (#43784) * Image URL Input: use new anchor prop for Popover * Prevent value from being `null` * Edit site Actions: use new anchor prop for Popover (#43810) * Buttons block: use new Popover anchor prop (#43785) * Buttons block: use new `anchor` prop for `Popover` * Prevent anchor value from being `null` * Navigation block: use new anchor prop for Popover (#43786) * Navigation block: use new `anchor` prop for `Popover` * Use anchor for the Navigation submenu block too * Prevent anchor value from being `null` * Post Date block: use new anchor prop for Popover (#43787) * Post Date block: use new `anchor` prop for `Popover` * Prevent anchor value from being `null` * Tooltip: refactor using Popover's new anchor prop (#43799) * Tooltip: use Popover s new anchor prop * Use internal state to force re-renders when the anchor ref changes * Simplify code * Improve docs around using state instead of refs for the anchor element * Allow `anchor` to be `null` * Edit Post: use Popover's new anchor prop (#43808) * Edit Post: use Popover s new anchor prop * Update comment * SImplify code * Update packages/edit-post/src/components/sidebar/post-schedule/index.js Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Allow passing a `null` anchor Co-authored-by: Daniel Richards <daniel.richards@automattic.com> * Refactor `useAnchorRef` and related components to work with the new Popover `anchor` prop (#43713) * useAnchorRef: return a VirtualElement instead of a range * Update useAnchorRef usage in FormatToolbarContainer, use anchor prop * Update remaining `useAnchorRef` usages, switch to the `anchor` prop * useAnchorRef: normalize `null` returns to `undefined` as it is not a valid `anchor` value * Revert changes to native RichText component * Update docs * Allow useAnchorRef to return `null` * Re-enable deprecation warnings * Remove fall back to `undefined` from `null` * Ensure reactive updates when the popover anchor updates * Refactor SocialLinkEdit component to use `anchor` instead of `anchorRef` * CHANGELOG * Add new `useAnchor` hook instead of changing existing `useAnchorRef` hook * Fix API docs * Update Popover unit tests * Remove unused import * Use DOMRect in the DomRectWithOwnerDocument type * Improve the wording of deprecation warnings * Put more emphasis on storing anchor in local state Co-authored-by: Lena Morita <lena@jaguchi.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com>
What?
Refactor the way the Dropdown component passes an anchor to Popover, using the new
anchor
propWhy?
See #43691 for more context
How?
When no anchor is being forwarded to
Popover
, use theanchor
prop instead ofanchorRef
Testing Instructions
Dropdown
storybook examples work without warningsDropdown
unit tests pass without warnings.Unit test failures caused by console warnings are expected. The reviews on this PR should focus on the specific refactor to
anchor
prop. This PR will be merged into #43691, so there will be another chance in that PR to give a final review.