Refactor useAnchorRef and related components to work with the new Popover anchor prop#43713
Merged
ciampo merged 7 commits intorefactor/popover-new-anchor-propfrom Sep 6, 2022
Conversation
ciampo
commented
Aug 30, 2022
Comment on lines
53
to
58
Contributor
Author
There was a problem hiding this comment.
This is the main change of the PR: this hook now returns a VirtualElement instead of a Range.
A VirtualElement is an element with the ownerDocument and getBoundingClientRect() properties, and is used by floating-ui to determine an anchor when there's not an actual DOM Element available.
Couple of notes:
- for the
getBoundingClientRect(), I decided to keep the same logic as currently in thePopover(i.e, just callgetBoundingClientRect()on the range). Alternatively, we could use the getRectangleFromRange utility, which was used in thePopovercomponent before the refactor happened in Refactor the Popover component to use the floatingUI library #40740 - for the
ownerDocument, I kept the same logic as the one from thePopovercomponent
ciampo
commented
Aug 30, 2022
Contributor
Author
There was a problem hiding this comment.
Other notable change in this PR: I decided to rename anchorRef to editableContentRef in this file, as it is better representing what that variable is for. editableContentRef is expected to be a "standard" React ref, ie. an object with the current property.
It looks like the FormatToolbarContainer is not exported from the package anyway, so I think this change is ok.
ciampo
commented
Aug 30, 2022
Contributor
Author
There was a problem hiding this comment.
Other change: because InlineToolbar now uses the Popover's anchor prop, we need to pass the actual element as the popoverAnchor prop (and not the reference)
ciampo
commented
Aug 30, 2022
|
Size Change: +34 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
7ce1a5e to
85077b0
Compare
85077b0 to
b65140f
Compare
76ee67c to
fb73356
Compare
5d38a79 to
64105f1
Compare
mirka
reviewed
Sep 1, 2022
Member
mirka
left a comment
There was a problem hiding this comment.
I can't pin down the exact repro conditions, but here are the steps I can take to reliably repro a crash bug when pasting an inline image:
- Make a new post with some text in a Paragraph block.
- Save as draft.
- Reload the editor.
- Paste an inline image.
CleanShot.2022-09-01.at.22.10.56.mp4
Although the crash point is in the BlockPopover component, it doesn't seem to reproduce in trunk so it could be related to this PR. Or, maybe something that a rebase will resolve.
Member
There was a problem hiding this comment.
Just checking — is it safe for this to return null? Should it be normalized to undefined? (I'm guessing the potential null comes from the element.closest().)
Contributor
Author
There was a problem hiding this comment.
I'm guessing the potential
nullcomes from theelement.closest()
Correct, that's why I had added null as a potential return value.
Your suggestion definitely makes sense to me, implemented in 7cea8ce
Contributor
Author
There was a problem hiding this comment.
Just flagging that, in the base PR, I've made a change to allow anchor to be null — this was in response to some feedback received on a related PR, where it was noted than allowing null as a possible value of the anchor would result in a better devX (ie. the Popover can handle the null case easily, instead of requiring its consumers to pass undefined instead of null, which is a typical value that refs can assume in React)
With that in mind, I've also pushed a change to this PR, restoring useAnchorRef's return value to include null too 1bc3b66
e29334e to
78a6127
Compare
78a6127 to
1de32a8
Compare
64105f1 to
7cea8ce
Compare
Contributor
Author
I should have hopefully fixed this problem in the base PR (67e35de), together with another fix (1de32a8) that was breaking the autocompleter component. Could you give this PR another look, please? |
…valid `anchor` value
bbea524 to
b179e5b
Compare
mirka
approved these changes
Sep 6, 2022
Member
mirka
left a comment
There was a problem hiding this comment.
Looks good, and the crash bug has been resolved 👍
ciampo
added a commit
that referenced
this pull request
Sep 6, 2022
…opover `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`
ciampo
added a commit
that referenced
this pull request
Sep 7, 2022
…opover `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`
ciampo
added a commit
that referenced
this pull request
Sep 9, 2022
…opover `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`
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
…opover `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`
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
…opover `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`
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
* 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>
ockham
pushed a commit
that referenced
this pull request
Sep 19, 2022
* 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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Refactor the return value of the
useAnchorRefhook, and refactor the components using that hook so that they use the newPopover'sanchorprop instead ofanchorRef.Why?
See #43691 for more context
How?
useAnchorRefso that is returns aVirualElementinstead of aRangeanchorprop instead ofanchorRefTesting Instructions
In the editor, test all popovers that are shown as a result of editing inline text (with and without selecting an explicit bit of text), and make sure that:
trunkPopovers:
@— a popover allowing you to pick from the list of existing users should appearUnit test failures caused by console warnings are expected. The reviews on this PR should focus on the specific refactor to
anchorprop. This PR will be merged into #43691, so there will be another chance in that PR to give a final review.