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

Edit site Actions: use new anchor prop for Popover #43810

Merged
merged 1 commit into from
Sep 5, 2022
Merged

Edit site Actions: use new anchor prop for Popover #43810

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 12 additions & 7 deletions packages/edit-site/src/components/header/document-actions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
__experimentalText as Text,
} from '@wordpress/components';
import { chevronDown } from '@wordpress/icons';
import { useRef } from '@wordpress/element';
import { useCallback, useState } from '@wordpress/element';
import { store as blockEditorStore } from '@wordpress/block-editor';

function getBlockDisplayText( block ) {
Expand Down Expand Up @@ -73,10 +73,15 @@ export default function DocumentActions( {
} ) {
const { label } = useSecondaryText();

// The title ref is passed to the popover as the anchorRef so that the
// dropdown is centered over the whole title area rather than just one
// part of it.
const titleRef = useRef();
// Use internal state instead of a ref to make sure that the component
// re-renders when then anchor's ref updates.
const [ popoverAnchor, setPopoverAnchor ] = useState();
const titleWrapperCallbackRef = useCallback( ( node ) => {
// Use the title wrapper as the popover anchor so that the dropdown is
// centered over the whole title area rather than just one part of it.
// Fall back to `undefined` in case the ref is `null`.
setPopoverAnchor( node ?? undefined );
}, [] );
Comment on lines +76 to +84
Copy link
Member

Choose a reason for hiding this comment

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

@ciampo, while I understand what this does. However, I'm not sure I fully understand why it's needed here 😅 Are there cases when the regular ref isn't set during the render?

Copy link
Contributor Author

@ciampo ciampo Sep 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I haven't tested this component extensively to see if there's any practical cases, but in theory:

  • titleRef would be undefined during the component's initial render
  • titleRef would update to the correct ref, but that wouldn't cause the component to re-render
  • if no other external factors caused the component to re-render, the Popover component would not receive an updated anchor

For this specific component, it would be entirely possible that the component re-renders multiple times for different reasons, but I still believe that this code change is a good idea:

  • it makes the component more resilient to future changes
  • it shows the correct pattern around making sure the anchor value is never stale, in case any other developer took it as an example

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @ciampo.

I think that's a good argument, and we can justify an extra render.


// Return a simple loading indicator until we have information to show.
if ( ! isLoaded ) {
Expand All @@ -103,7 +108,7 @@ export default function DocumentActions( {
} ) }
>
<div
ref={ titleRef }
ref={ titleWrapperCallbackRef }
className="edit-site-document-actions__title-wrapper"
>
<Text
Expand Down Expand Up @@ -131,7 +136,7 @@ export default function DocumentActions( {
{ dropdownContent && (
<Dropdown
popoverProps={ {
anchorRef: titleRef.current,
anchor: popoverAnchor,
} }
position="bottom center"
renderToggle={ ( { isOpen, onToggle } ) => (
Expand Down