Block inbetween inserter: use Popover's new anchor prop#43693
Merged
ciampo merged 3 commits intorefactor/popover-new-anchor-propfrom Aug 31, 2022
Merged
Block inbetween inserter: use Popover's new anchor prop#43693ciampo merged 3 commits intorefactor/popover-new-anchor-propfrom
ciampo merged 3 commits intorefactor/popover-new-anchor-propfrom
Conversation
|
Size Change: -56 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
commented
Aug 30, 2022
| const popoverAnchor = useMemo( () => { | ||
| if ( ( ! previousElement && ! nextElement ) || ! isVisible ) { | ||
| return {}; | ||
| return undefined; |
Contributor
Author
There was a problem hiding this comment.
Change no. 1: return undefined instead of an empty object, since that wouldn't be a valid value for the anchor prop
ciampo
commented
Aug 30, 2022
| bottom: previousRect ? previousRect.bottom : nextRect.bottom, | ||
| height: 0, | ||
| width: 0, | ||
| ownerDocument, |
Contributor
Author
There was a problem hiding this comment.
Change no.2 : moved ownerDocument to a top-level prop, sibling of getBoundingClientRect()
ciampo
commented
Aug 30, 2022
Comment on lines
+103
to
+137
| getBoundingClientRect() { | ||
| const prevRect = previousElement | ||
| ? previousElement.getBoundingClientRect() | ||
| : null; | ||
| const nextRect = nextElement | ||
| ? nextElement.getBoundingClientRect() | ||
| : null; | ||
|
|
||
| let left = 0; | ||
| let top = 0; | ||
|
|
||
| if ( isVertical ) { | ||
| // vertical | ||
| top = prevRect ? prevRect.bottom : nextRect.top; | ||
|
|
||
| if ( isRTL() ) { | ||
| // vertical, rtl | ||
| left = prevRect ? prevRect.right : nextRect.right; | ||
| } else { | ||
| // vertical, ltr | ||
| left = prevRect ? prevRect.left : nextRect.left; | ||
| } | ||
| } else { | ||
| top = prevRect ? prevRect.top : nextRect.top; | ||
|
|
||
| if ( isRTL() ) { | ||
| // non vertical, rtl | ||
| left = prevRect ? prevRect.left : nextRect.right; | ||
| } else { | ||
| // non vertical, ltr | ||
| left = prevRect ? prevRect.right : nextRect.left; | ||
| } | ||
| } | ||
|
|
||
| return new window.DOMRect( left, top, 0, 0 ); |
Contributor
Author
There was a problem hiding this comment.
Change no. 2: moved the remaining logic for creating a rect to the getBoundingClientRect() property.
The main two changes here are:
- Return a proper
DOMRectobject, instead of a manually created one - Simplify the logic by removing unnecessary calculations
The removal of unnecessary calculations is based on the fact that currently the resulting rect would always have a 0 width and height anyway! This is backed by a few reasons:
- the current implementation on
trunkalways setswidthandheightto0, which take the priority in thePopovercomponent when computing the final anchor rect - even before the refactor in Refactor the Popover component to use the floatingUI library #40740, the
Popovercomponent used to disregard therightandbottomprops when calculating its anchor
Even before the refactor
ciampo
commented
Aug 30, 2022
| }, | ||
| }; | ||
| }, [ previousElement, nextElement ] ); | ||
| }, [ previousElement, nextElement, isVisible ] ); |
Contributor
Author
There was a problem hiding this comment.
Change no. 3: add isVisible to the list of dependencies
f72a588 to
fab0cf5
Compare
76ee67c to
fb73356
Compare
fab0cf5 to
4351685
Compare
mirka
approved these changes
Aug 31, 2022
Member
mirka
left a comment
There was a problem hiding this comment.
I verified that the isVertical/isRTL conditionals remain the same, and it works as expected in the editor 👍
| }, | ||
| }; | ||
| }, [ previousElement, nextElement ] ); | ||
| }, [ previousElement, nextElement, isVisible ] ); |
ciampo
added a commit
that referenced
this pull request
Sep 1, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
ciampo
added a commit
that referenced
this pull request
Sep 6, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
ciampo
added a commit
that referenced
this pull request
Sep 7, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
ciampo
added a commit
that referenced
this pull request
Sep 9, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
* BlockPopoverInbetween: refactor to use `anchor` prop * Simplify logic, use DOMRect * Add missing hook deps
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
2 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 way the block inbetween inserter Popover's anchor is defined, using the new
anchorpropWhy?
See #43691 for more context
How?
The logic for showing the popover is the same, but is now part of the block toolbar component.
Testing Instructions
In the editor, hover over the space inbetween blocks, in order to trigger to cause the inbetween inserter to appear and make sure that:
trunkUnit 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.