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

Jump to bottom

Block inbetween inserter: use Popover's new anchor prop #43693

Merged
merged 3 commits into from
Aug 31, 2022
Merged

Block inbetween inserter: use Popover's new anchor prop #43693

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a265060
BlockPopoverInbetween: refactor to use `anchor` prop
ciampo Aug 30, 2022
4351685
Simplify logic, use DOMRect
ciampo Aug 30, 2022
4d6926a
Add missing hook deps
ciampo Aug 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
95 changes: 41 additions & 54 deletions packages/block-editor/src/components/block-popover/inbetween.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { useCallback, useMemo, createContext } from '@wordpress/element';
import { useMemo, createContext } from '@wordpress/element';
import { Popover } from '@wordpress/components';
import { isRTL } from '@wordpress/i18n';

Expand Down Expand Up @@ -91,66 +91,53 @@ function BlockPopoverInbetween( {
};
}, [ previousElement, nextElement, isVertical ] );

const getAnchorRect = useCallback( () => {
const popoverAnchor = useMemo( () => {
if ( ( ! previousElement && ! nextElement ) || ! isVisible ) {
return {};
return undefined;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change no. 1: return undefined instead of an empty object, since that wouldn't be a valid value for the anchor prop

}

const { ownerDocument } = previousElement || nextElement;

const previousRect = previousElement
? previousElement.getBoundingClientRect()
: null;
const nextRect = nextElement
? nextElement.getBoundingClientRect()
: null;

if ( isVertical ) {
if ( isRTL() ) {
return {
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.right : nextRect.right,
right: previousRect ? previousRect.left : nextRect.left,
bottom: nextRect ? nextRect.top : previousRect.bottom,
height: 0,
width: 0,
ownerDocument,
};
}

return {
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.left : nextRect.left,
right: previousRect ? previousRect.right : nextRect.right,
bottom: nextRect ? nextRect.top : previousRect.bottom,
height: 0,
width: 0,
ownerDocument,
};
}

if ( isRTL() ) {
return {
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.left : nextRect.right,
right: nextRect ? nextRect.right : previousRect.left,
bottom: previousRect ? previousRect.bottom : nextRect.bottom,
height: 0,
width: 0,
ownerDocument,
};
}

return {
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.right : nextRect.left,
right: nextRect ? nextRect.left : previousRect.right,
bottom: previousRect ? previousRect.bottom : nextRect.bottom,
height: 0,
width: 0,
ownerDocument,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change no.2 : moved ownerDocument to a top-level prop, sibling of getBoundingClientRect()

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 );
Comment on lines +103 to +137
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change no. 2: moved the remaining logic for creating a rect to the getBoundingClientRect() property.

The main two changes here are:

  1. Return a proper DOMRect object, instead of a manually created one
  2. 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:

},
};
}, [ previousElement, nextElement ] );
}, [ previousElement, nextElement, isVisible ] );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change no. 3: add isVisible to the list of dependencies

Copy link
Member

Choose a reason for hiding this comment

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

How about isVertical?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙈

4d6926a


const popoverScrollRef = usePopoverScroll( __unstableContentRef );

Expand All @@ -172,7 +159,7 @@ function BlockPopoverInbetween( {
<Popover
ref={ popoverScrollRef }
animate={ false }
getAnchorRect={ getAnchorRect }
anchor={ popoverAnchor }
focusOnMount={ false }
// Render in the old slot if needed for backward compatibility,
// otherwise render in place (not in the default popover slot).
Expand Down