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.

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

Format Library: Assign Popover anchorRect to update selection position #14938

Merged
merged 8 commits into from
Apr 12, 2019
1 change: 1 addition & 0 deletions packages/block-editor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- Added the usage of `mediaPreview` for the `Placeholder` component to the `MediaPlaceholder` component.
- Added a an `onDoubleClick` event handler to the `MediaPlaceholder` component.
- Added a way to pass special `ref` property to the `PlainText` component.
- The `URLPopover` component now passes through all unhandled props to the underlying Popover component.

### Breaking Changes

Expand Down
10 changes: 1 addition & 9 deletions packages/block-editor/src/components/url-popover/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class MyURLPopover extends Component {

## Props

The component accepts the following props.
The component accepts the following props. Any other props are passed through to the underlying `Popover` component ([refer to props documentation](/packages/components/src/popover/README.md)).

### position

Expand All @@ -104,14 +104,6 @@ an element.
- Required: No
- Default: "firstElement"

### onClose

Callback that triggers when the user indicates the popover should close (e.g. they've used the escape key or clicked
outside of the popover.)

- Type: `Function`
- Required: No

### renderSettings

Callback used to return the React Elements that will be rendered inside the settings drawer. When this function
Expand Down
6 changes: 2 additions & 4 deletions packages/block-editor/src/components/url-popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,9 @@ class URLPopover extends Component {
const {
children,
renderSettings,
onClose,
onClickOutside,
position = 'bottom center',
focusOnMount = 'firstElement',
...popoverProps
Copy link
Member

Choose a reason for hiding this comment

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

I think I reviewed it already earlier this week 😃

} = this.props;

const {
Expand All @@ -46,8 +45,7 @@ class URLPopover extends Component {
className="editor-url-popover block-editor-url-popover"
focusOnMount={ focusOnMount }
position={ position }
onClose={ onClose }
onClickOutside={ onClickOutside }
{ ...popoverProps }
>
<div className="editor-url-popover__row block-editor-url-popover__row">
{ children }
Expand Down
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
- Added a new `render` property to `FormFileUpload` component. Allowing users of the component to custom the UI for their needs.
- Added a new `BaseControl.VisualLabel` component.
- Added a new `preview` prop to the `Placeholder` component which allows to display a preview, for example a media preview when the Placeholder is used in media editing contexts.
- Added a new `anchorRect` prop to `Popover` which enables a developer to provide a custom `DOMRect` object at which to position the popover.

### Bug fixes

Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ export { default as PanelHeader } from './panel/header';
export { default as PanelRow } from './panel/row';
export { default as Placeholder } from './placeholder';
export { default as Popover } from './popover';
export { default as PositionedAtSelection } from './positioned-at-selection';
export { default as __unstablePositionedAtSelection } from './positioned-at-selection';
export { default as QueryControls } from './query-controls';
export { default as RadioControl } from './radio-control';
export { default as RangeControl } from './range-control';
Expand Down
7 changes: 7 additions & 0 deletions packages/components/src/popover/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,13 @@ Opt-in prop to show popovers fullscreen on mobile, pass `false` in this prop to
- Required: No
- Default: `false`

### anchorRect

A custom `DOMRect` object at which to position the popover.

- Type: `DOMRect`
- Required: No

## Methods

### refresh
Expand Down
27 changes: 22 additions & 5 deletions packages/components/src/popover/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ class Popover extends Component {
}

componentDidMount() {
this.toggleAutoRefresh( true );
this.toggleAutoRefresh( ! this.props.hasOwnProperty( 'anchorRect' ) );
this.refresh();

/*
Expand All @@ -87,6 +87,15 @@ class Popover extends Component {
if ( prevProps.position !== this.props.position ) {
this.computePopoverPosition( this.state.popoverSize, this.anchorRect );
}

if ( prevProps.anchorRect !== this.props.anchorRect ) {
this.refreshOnAnchorMove();
}

const hasAnchorRect = this.props.hasOwnProperty( 'anchorRect' );
if ( hasAnchorRect !== prevProps.hasOwnProperty( 'anchorRect' ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why use hasOwnProperty?

Copy link
Member Author

@aduth aduth Apr 12, 2019

Choose a reason for hiding this comment

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

Why use hasOwnProperty?

Because we expect the value could change over time, but scheduling the interval doesn't depend on the value, it just depends on whether a value exists.

i.e. if the alternative is this.props.anchorRect !== prevProps.anchorRect, it would have unintended consequences on the auto-refresh.

this.toggleAutoRefresh( ! hasAnchorRect );
}
}

componentWillUnmount() {
Expand Down Expand Up @@ -129,8 +138,7 @@ class Popover extends Component {
* will only refresh the popover position if the anchor moves.
*/
refreshOnAnchorMove() {
const { getAnchorRect = this.getAnchorRect } = this.props;
const anchorRect = getAnchorRect( this.anchorNode.current );
const anchorRect = this.getAnchorRect( this.anchorNode.current );
const didAnchorRectChange = ! isShallowEqual( anchorRect, this.anchorRect );
if ( didAnchorRectChange ) {
this.anchorRect = anchorRect;
Expand All @@ -144,8 +152,7 @@ class Popover extends Component {
* position.
*/
refresh() {
const { getAnchorRect = this.getAnchorRect } = this.props;
const anchorRect = getAnchorRect( this.anchorNode.current );
const anchorRect = this.getAnchorRect( this.anchorNode.current );
const contentRect = this.contentNode.current.getBoundingClientRect();
const popoverSize = {
width: contentRect.width,
Expand Down Expand Up @@ -191,6 +198,16 @@ class Popover extends Component {
}

getAnchorRect( anchor ) {
const { getAnchorRect, anchorRect } = this.props;

if ( anchorRect ) {
return anchorRect;
}

if ( getAnchorRect ) {
return getAnchorRect( anchor );
}

if ( ! anchor || ! anchor.parentNode ) {
return;
}
Expand Down
6 changes: 3 additions & 3 deletions packages/format-library/src/image/index.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { Path, SVG, TextControl, Popover, IconButton, PositionedAtSelection } from '@wordpress/components';
import { Path, SVG, TextControl, Popover, IconButton, __unstablePositionedAtSelection } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { Component } from '@wordpress/element';
import { insertObject } from '@wordpress/rich-text';
Expand Down Expand Up @@ -113,7 +113,7 @@ export const image = {
return null;
} }
/> }
{ isObjectActive && <PositionedAtSelection key={ key }>
{ isObjectActive && <__unstablePositionedAtSelection key={ key }>
<Popover
position="bottom center"
focusOnMount={ false }
Expand Down Expand Up @@ -155,7 +155,7 @@ export const image = {
</form>
{ /* eslint-enable jsx-a11y/no-noninteractive-element-interactions */ }
</Popover>
</PositionedAtSelection> }
</__unstablePositionedAtSelection> }
</MediaUploadCheck>
);
}
Expand Down
96 changes: 64 additions & 32 deletions packages/format-library/src/link/inline.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,15 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Component, createRef } from '@wordpress/element';
import { Component, createRef, useMemo } from '@wordpress/element';
import {
ExternalLink,
IconButton,
ToggleControl,
withSpokenMessages,
PositionedAtSelection,
} from '@wordpress/components';
import { LEFT, RIGHT, UP, DOWN, BACKSPACE, ENTER } from '@wordpress/keycodes';
import { getRectangleFromRange } from '@wordpress/dom';
import { prependHTTP, safeDecodeURI, filterURLForDisplay } from '@wordpress/url';
import {
create,
Expand Down Expand Up @@ -77,6 +77,39 @@ const LinkViewerUrl = ( { url } ) => {
);
};

const URLPopoverAtLink = ( { isActive, addingLink, value, ...props } ) => {
const anchorRect = useMemo( () => {
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering... Does this have a limit on cache size? Sounds like it should have a limit of exactly one.

Copy link
Member Author

@aduth aduth Apr 12, 2019

Choose a reason for hiding this comment

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

Just wondering... Does this have a limit on cache size? Sounds like it should have a limit of exactly one.

It's not entirely clear, but it seems like something they would have considered as being the default behavior, but I also recall some similar discussion with @gziolo recently where he was showing a separate project which, by its name, might imply a different behavior 🤷‍♂️

https://reactjs.org/docs/hooks-reference.html#usememo

You may rely on useMemo as a performance optimization, not as a semantic guarantee. In the future, React may choose to “forget” some previously memoized values and recalculate them on next render, e.g. to free memory for offscreen components. Write your code so that it still works without useMemo — and then add it to optimize performance.

https://github.com/alexreardon/use-memo-one#readme

useMemo and useCallback cache the most recent result. However, this cache can be destroyed by React when it wants to:

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, to me it sounded like it was not the intention of useMemo to limit the cache to one by default. So I would consider swapping it. I'm generally wary of things that create cache without any limit. We're just taking more and more space from memory for no reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, to me it sounded like it was not the intention of useMemo to limit the cache to one by default. So I would consider swapping it. I'm generally wary of things that create cache without any limit. We're just taking more and more space from memory for no reason.

Oh, I was thinking the opposite by my interpretation; that yes, it would only store the one latest result in memory. I'll plan to double-check, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

const range = window.getSelection().getRangeAt( 0 );
if ( ! range ) {
return;
}

if ( addingLink ) {
return getRectangleFromRange( range );
}

let element = range.startContainer;

// If the caret is right before the element, select the next element.
element = element.nextElementSibling || element;

while ( element.nodeType !== window.Node.ELEMENT_NODE ) {
element = element.parentNode;
}

const closest = element.closest( 'a' );
if ( closest ) {
return closest.getBoundingClientRect();
}
}, [ isActive, addingLink, value.start, value.end ] );
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know that isActive needed to be listed as a dependency. It's not ever used in the useMemo callback.


if ( ! anchorRect ) {
return null;
}

return <URLPopover anchorRect={ anchorRect } { ...props } />;
};

const LinkViewer = ( { url, editLink } ) => {
return (
// Disable reason: KeyPress must be suppressed so the block doesn't hide the toolbar
Expand Down Expand Up @@ -221,37 +254,36 @@ class InlineLinkUI extends Component {
const showInput = isShowingInput( this.props, this.state );

return (
<PositionedAtSelection
ellatrix marked this conversation as resolved.
Show resolved Hide resolved
key={ `${ value.start }${ value.end }` /* Used to force rerender on selection change */ }
<URLPopoverAtLink
value={ value }
isActive={ isActive }
addingLink={ addingLink }
onClickOutside={ this.onClickOutside }
onClose={ this.resetState }
focusOnMount={ showInput ? 'firstElement' : false }
renderSettings={ () => (
<ToggleControl
label={ __( 'Open in New Tab' ) }
checked={ opensInNewWindow }
onChange={ this.setLinkTarget }
/>
) }
>
<URLPopover
onClickOutside={ this.onClickOutside }
onClose={ this.resetState }
focusOnMount={ showInput ? 'firstElement' : false }
renderSettings={ () => (
<ToggleControl
label={ __( 'Open in New Tab' ) }
checked={ opensInNewWindow }
onChange={ this.setLinkTarget }
/>
) }
>
{ showInput ? (
<LinkEditor
value={ inputValue }
onChangeInputValue={ this.onChangeInputValue }
onKeyDown={ this.onKeyDown }
submitLink={ this.submitLink }
autocompleteRef={ this.autocompleteRef }
/>
) : (
<LinkViewer
url={ url }
editLink={ this.editLink }
/>
) }
</URLPopover>
</PositionedAtSelection>
{ showInput ? (
<LinkEditor
value={ inputValue }
onChangeInputValue={ this.onChangeInputValue }
onKeyDown={ this.onKeyDown }
submitLink={ this.submitLink }
autocompleteRef={ this.autocompleteRef }
/>
) : (
<LinkViewer
url={ url }
editLink={ this.editLink }
/>
) }
</URLPopoverAtLink>
);
}
}
Expand Down