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

Image block: fix resize listener #22277

Merged
merged 3 commits into from
May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
53 changes: 28 additions & 25 deletions packages/block-library/src/image/image-size.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
/**
* WordPress dependencies
*/
import { withGlobalEvents } from '@wordpress/compose';
import { useGlobalEvent } from '@wordpress/compose';
import { useRef, useState, useEffect } from '@wordpress/element';

/**
* Internal dependencies
*/
import { calculatePreferedImageSize } from './utils';

function ImageSize( { src, dirtynessTrigger, children } ) {
export default function ImageSize( { src, dirtynessTrigger, children } ) {
const ref = useRef();
const image = useRef();
const [ state, setState ] = useState( {
imageWidth: null,
imageHeight: null,
Expand All @@ -20,35 +21,37 @@ function ImageSize( { src, dirtynessTrigger, children } ) {
imageHeightWithinContainer: null,
} );

function calculateSize() {
const { width, height } = calculatePreferedImageSize(
image.current,
ref.current
);

setState( {
imageWidth: image.current.width,
imageHeight: image.current.height,
containerWidth: ref.current.clientWidth,
containerHeight: ref.current.clientHeight,
imageWidthWithinContainer: width,
imageHeightWithinContainer: height,
} );
}

useEffect( () => {
const image = new window.Image();

image.onload = () => {
const { width, height } = calculatePreferedImageSize(
image,
ref.current
);

setState( {
imageWidth: image.width,
imageHeight: image.height,
containerWidth: ref.current.clientWidth,
containerHeight: ref.current.clientHeight,
imageWidthWithinContainer: width,
imageHeightWithinContainer: height,
} );
};
if ( ! image.current ) {
const { defaultView } = ref.current.ownerDocument;
image.current = new defaultView.Image();
}

image.src = src;
image.current.onload = calculateSize;
image.current.src = src;

return () => {
image.onload = undefined;
image.current.onload = undefined;
};
}, [ src, dirtynessTrigger ] );

useGlobalEvent( ref, [ 'resize', calculateSize ], [] );

return <div ref={ ref }>{ children( state ) }</div>;
}

export default withGlobalEvents( {
resize: 'calculateSize',
} )( ImageSize );
10 changes: 10 additions & 0 deletions packages/compose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,16 @@ _Returns_

- `WPComponent`: Component class with generated display name assigned.

<a name="useGlobalEvent" href="#useGlobalEvent">#</a> **useGlobalEvent**

Adds a listener to the containing window.

_Parameters_

- _ref_ `Object`: A reference with a node that is contained by the window.
- _args_ `Array`: `addEventListener` arguments.
- _dependencies_ `Array`: Hook dependencies.

<a name="useInstanceId" href="#useInstanceId">#</a> **useInstanceId**

Provides a unique instance ID.
Expand Down
24 changes: 24 additions & 0 deletions packages/compose/src/hooks/use-global-event/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* WordPress dependencies
*/
import { useEffect } from '@wordpress/element';

/**
* Adds a listener to the containing window.
*
* @param {Object} ref A reference with a node that is contained by the
* window.
* @param {Array} args `addEventListener` arguments.
* @param {Array} dependencies Hook dependencies.
*/
export default function useGlobalEvent( ref, args, dependencies ) {
Copy link
Contributor

@youknowriad youknowriad May 12, 2020

Choose a reason for hiding this comment

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

One of the reasons for the existence of withGlobalEvent is the performance impact of using a single event listener for multiple components using the same args. Do we want to support that here?

Copy link
Member

@aduth aduth May 12, 2020

Choose a reason for hiding this comment

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

One of the reasons for the existence of withGlobalEvent is the performance impact of using a single event listener for multiple components using the same args. Do we want to support that here?

I forget where exactly I'd remarked about it, but over time I started to question whether there really should expect to be any performance benefit of using one event handler for all global events, since in either case something would need to iterate and invoke each of the handlers. It seems browser native code could likely be more efficient about this.

Copy link
Member

Choose a reason for hiding this comment

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

I forget where exactly I'd remarked about it

See #18816 (comment)

useEffect( () => {
const { defaultView } = ref.current.ownerDocument;

defaultView.addEventListener( ...args );

return () => {
defaultView.removeEventListener( ...args );
};
}, dependencies );
}
1 change: 1 addition & 0 deletions packages/compose/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export { default as withState } from './higher-order/with-state';

// Hooks
export { default as __experimentalUseDragging } from './hooks/use-dragging';
export { default as useGlobalEvent } from './hooks/use-global-event';
export { default as useInstanceId } from './hooks/use-instance-id';
export { default as useKeyboardShortcut } from './hooks/use-keyboard-shortcut';
export { default as useMediaQuery } from './hooks/use-media-query';
Expand Down