BorderBoxControl: use Popover's new anchor prop #43789
Merged
Conversation
This file contains 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
ciampo
requested review from
mirka,
ntsekouras,
talldan,
aaronrobertshaw and
chad1008
ciampo
added
[Type] Enhancement
ciampo
commented
Sep 1, 2022
Comment on lines
39
to
57
| const [ popoverProps, setPopoverProps ] = useState< PopoverPartialProps >(); | ||
| const [ popoverAnchor, setPopoverAnchor ] = useState< Element >(); | ||
|
|
||
| const containerRef = useCallback( ( node ) => { | ||
| setPopoverAnchor( node ); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| if ( popoverPlacement ) { | ||
| setPopoverProps( { | ||
| placement: popoverPlacement, | ||
| offset: popoverOffset, | ||
| anchorRef: containerRef, | ||
| anchor: popoverAnchor, | ||
| __unstableShift: true, | ||
| } | ||
| : undefined; | ||
| } ); | ||
| } else { | ||
| setPopoverProps( undefined ); | ||
| } | ||
| }, [ popoverPlacement, popoverOffset, popoverAnchor ] ); |
There was a problem hiding this comment.
This changes are in place to make sure that, when the containerRef is updated, popoverProps are updated too.
Without it, the ref would be updated after the initial value of popoverProps was computed, and because of how React refs work, React wouldn't re-render the component — meaning that the anchor property in popoverProps would be stale until a re-render would be caused by whatever reason.
ciampo
commented
Sep 1, 2022
| placement: popoverPlacement, | ||
| offset: popoverOffset, | ||
| anchorRef: containerRef, | ||
| anchor: popoverAnchor, |
There was a problem hiding this comment.
Using anchor instead of anchorRef
ciampo
commented
Sep 1, 2022
Comment on lines
66
to
86
| const [ popoverProps, setPopoverProps ] = useState< PopoverPartialProps >(); | ||
| const [ popoverAnchor, setPopoverAnchor ] = useState< Element >(); | ||
|
|
||
| const containerRef = useCallback( ( node ) => { | ||
| setPopoverAnchor( node ); | ||
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| if ( popoverPlacement ) { | ||
| setPopoverProps( { | ||
| placement: popoverPlacement, | ||
| offset: popoverOffset, | ||
| anchorRef: containerRef, | ||
| anchor: popoverAnchor, | ||
| __unstableShift: true, | ||
| } | ||
| : undefined; | ||
| } ); | ||
| } else { | ||
| setPopoverProps( undefined ); | ||
| } | ||
| }, [ popoverPlacement, popoverOffset, popoverAnchor ] ); | ||
|
|
||
| const mergedRef = useMergeRefs( [ containerRef, forwardedRef ] ); |
There was a problem hiding this comment.
Same set of changes as in the other component
ciampo
commented
Sep 1, 2022
Comment on lines
+24
to
+29
| export type PopoverPartialProps = { | ||
| placement?: Placement; | ||
| offset?: number; | ||
| anchor?: ReferenceType; | ||
| __unstableShift?: boolean; | ||
| }; |
There was a problem hiding this comment.
This should be removed and replaced with the original PopoverProps type once we type the component (hopefully soon!)
ciampo
commented
Sep 1, 2022
| @@ -85,6 +85,7 @@ Default.args = { | |||
| width: '1px', | |||
| }, | |||
| __next36pxDefaultSize: false, | |||
| popoverPlacement: 'right-start', | |||
There was a problem hiding this comment.
I added popoverPlacement while testing for the stale anchor value, and decided to keep it for convenience.
But the component's stories definitely need to be rewritten in TypeScript so that they can show all props
There was a problem hiding this comment.
I have it on my radar to rewrite the border control component stories in TypeScript and update them to follow the latest approach and guidelines. Unfortunately, I haven't had the bandwidth in the lead-up to 6.1 but hope to in the not-too-distant future.
ciampo
commented
Sep 1, 2022
| }, [] ); | ||
|
|
||
| useEffect( () => { | ||
| if ( popoverPlacement ) { |
There was a problem hiding this comment.
Side note: I'm not in love with the popoverPlacement and popoverOffset props, but these were added as part of #40740 and it's not in the scope of this PR to revisit the APIs of BorderBoxControl
|
Size Change: +45 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Sep 5, 2022
There was a problem hiding this comment.
Nice work @ciampo ✨
The changes look good and tested well for me.
✅ Popover appears as expected when clicking on color button
✅ Popover position updates appropriately when adjusting popover placement in Storybook
✅ When no popover placement is set color button is used as the anchor
✅ Split controls and their popovers behave consistently with the linked control
✅ Popover placement between this PR and trunk is consistent in the editor
Storybook video
Screen.Recording.2022-09-05.at.7.03.02.pm.mp4
Editor video
Screen.Recording.2022-09-05.at.7.29.10.pm.mp4
P.S. We'll need a changelog 🙂
|
Thank you for the review, @aaronrobertshaw !
Good call. I'm planning on adding CHANGELOG entries on the base PR before merging to |
ciampo
added a commit
that referenced
this pull request
Sep 6, 2022
* BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null`
ciampo
added a commit
that referenced
this pull request
Sep 7, 2022
* BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null`
ciampo
added a commit
that referenced
this pull request
Sep 9, 2022
* BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null`
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
* BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `null`
ciampo
added a commit
that referenced
this pull request
Sep 14, 2022
* BorderBoxControl: use new `anchor` prop for `Popover` * Make sure anchor value is `undefined` instead of `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>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
BorderBoxControlcomponent passes an anchor to Popover, using the newanchorpropWhy?
See #43691 for more context
How?
anchorRefwithanchorTesting Instructions
In Storybook:
Open the
BorderBoxControlcomponent and:popoverPlacementprop and make sure that the popover's position updates as expected when changing that proppopoverPlacementis set, the popover uses the control group as its anchorpopoverPlacementis not set, the popover uses the icon button as its anchorpopoverPlacementis set, the popover uses the control group as its anchorpopoverPlacementis not set, the popover uses each individual icon button as its anchorIn the editor:
BorderBoxControlcomponenttrunkborder-box-control.mp4