From 9a0ab20c48c8537325046a3fdec881a948c191d8 Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Wed, 18 Dec 2024 09:18:11 +0000 Subject: [PATCH 1/2] refactor: image loading handlers Some changes to clean up image loading code, and speed up subject image loading. - lib-classifier: remove `useEffect` from `useSubjectImage`, so that image loading code runs on image load, rather than waiting for the next React render cycle. - lib-react-components: when there's no delay for `useProgressiveImage`, run the `onLoad` handler immediately, rather than waiting for the next event cycle. - refactor `onLoad` event handlers to use `load` events, rather than `Image` objects. This brings the event handlers in line with the DOM standard. --- .../src/hooks/useSubjectImage.js | 17 +++--- packages/lib-react-components/CHANGELOG.md | 1 + .../ThumbnailImage/ThumbnailImage.spec.js | 6 ++- .../lib-react-components/src/hooks/Readme.md | 11 +++- .../src/hooks/useProgressiveImage.js | 54 +++++++++++++------ 5 files changed, 59 insertions(+), 30 deletions(-) diff --git a/packages/lib-classifier/src/hooks/useSubjectImage.js b/packages/lib-classifier/src/hooks/useSubjectImage.js index ffda912dfa..e3ac2a7e74 100644 --- a/packages/lib-classifier/src/hooks/useSubjectImage.js +++ b/packages/lib-classifier/src/hooks/useSubjectImage.js @@ -6,12 +6,14 @@ export default function useSubjectImage({ src, onReady, onError }) { const { img, error, loading } = useProgressiveImage({ placeholderSrc: '', - src + src, + onLoad, + onError }) - useEffect(function onImageLoad() { - const { naturalHeight, naturalWidth, src } = img + function onLoad(event) { + const { naturalHeight, naturalWidth, src } = event.target if (src !== '' ) { const svgImage = subjectImage.current const { width: clientWidth, height: clientHeight } = svgImage @@ -20,14 +22,7 @@ export default function useSubjectImage({ src, onReady, onError }) { const target = { clientHeight, clientWidth, naturalHeight, naturalWidth } onReady({ target }) } - }, [img, onReady, subjectImage]) - - useEffect(function logError() { - if (!loading && error) { - console.error(error) - onError(error) - } - }, [error, loading, onError]) + } return { img, error, loading, subjectImage } } diff --git a/packages/lib-react-components/CHANGELOG.md b/packages/lib-react-components/CHANGELOG.md index 74b649f1d6..b5dbdac728 100644 --- a/packages/lib-react-components/CHANGELOG.md +++ b/packages/lib-react-components/CHANGELOG.md @@ -13,6 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - PlainButton has text-decoration underline on hover even for links. - ZooFooter links and labels updated to reflect newly launched FEM pages. - Updated styling in ProjectCard's badge component. +- `useProgressiveImage`: run `onLoad` callbacks immediately when `delay` is 0. Add optional `onLoad` and `onError` event handlers for image `load` events. ### Fixed diff --git a/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.spec.js b/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.spec.js index f087ff9a5a..01fcf30f68 100644 --- a/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.spec.js +++ b/packages/lib-react-components/src/Media/components/ThumbnailImage/ThumbnailImage.spec.js @@ -12,7 +12,11 @@ describe('ThumbnailImage', function () { constructor () { this.naturalHeight = 200 this.naturalWidth = 400 - setTimeout(() => this.onload(), 0) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), 0) } } diff --git a/packages/lib-react-components/src/hooks/Readme.md b/packages/lib-react-components/src/hooks/Readme.md index 6f54362ab1..91b5fe116c 100644 --- a/packages/lib-react-components/src/hooks/Readme.md +++ b/packages/lib-react-components/src/hooks/Readme.md @@ -47,8 +47,17 @@ const { data: user, error, isLoading } = usePanoptesUser() Use a placeholder while an image downloads. ```jsx +function onLoad(event) { + console.log('image loaded: ', event.target) +} + +function onError(error) { + console.warn('loading failed') + console.error(error) +} + const src = 'https://panoptes-uploads.zooniverse.org/production/subject_location/66094a64-8823-4314-8ef4-1ee228e49470.jpeg' -const { img, error, loading } = useProgressiveImage({ delay: 0, src }) +const { img, error, loading } = useProgressiveImage({ delay: 0, src, onLoad, onError }) return This is an example of an image with a placeholder. ``` diff --git a/packages/lib-react-components/src/hooks/useProgressiveImage.js b/packages/lib-react-components/src/hooks/useProgressiveImage.js index 5c28d35189..34f0bb0509 100644 --- a/packages/lib-react-components/src/hooks/useProgressiveImage.js +++ b/packages/lib-react-components/src/hooks/useProgressiveImage.js @@ -1,4 +1,4 @@ -import { useEffect, useState } from 'react' +import { useEffect, useRef, useState } from 'react' const defaultPlaceholder = { naturalHeight: 600, @@ -6,35 +6,55 @@ const defaultPlaceholder = { src: '' } +const DEFAULT_HANDLER = () => false + +function preloadImage({ + src, + onLoad = DEFAULT_HANDLER, + onError = DEFAULT_HANDLER +}) { + const { Image } = window + const img = new Image() + img.onload = onLoad + img.onerror = onError + img.src = src +} + export default function useProgressiveImage({ delay = 0, placeholderSrc = '', - src + src, + onLoad = DEFAULT_HANDLER, + onError = DEFAULT_HANDLER }) { const placeholder = { ...defaultPlaceholder, src: placeholderSrc } const [loading, setLoading] = useState(true) - const [img, setImg] = useState(placeholder) + const imgRef = useRef(placeholder) const [error, setError] = useState(null) + function onImageLoad(event) { + imgRef.current = event.target + onLoad(event) + setLoading(false) + } + + function onImageError(error) { + onError(error) + setError(error) + setLoading(false) + } + function fetchImage() { - const { Image } = window - const img = new Image() - img.onload = () => { - setTimeout(() => { - setImg(img) - setLoading(false) - }, delay) - } - img.onerror = (error) => { - setError(error) - setLoading(false) - } setLoading(true) setError(null) - img.src = src + preloadImage({ + src, + onLoad: delay ? event => setTimeout(onImageLoad, delay, event) : onImageLoad, + onError: onImageError + }) } useEffect(function onNewImage() { @@ -43,5 +63,5 @@ export default function useProgressiveImage({ } }, [src]) - return { img, error, loading } + return { img: imgRef.current, error, loading } } From 3f561d1a947d935b34bef5f4b39fe6af74732ce8 Mon Sep 17 00:00:00 2001 From: Jim O'Donnell Date: Wed, 18 Dec 2024 11:59:33 +0000 Subject: [PATCH 2/2] add mock load events to classifier tests --- .../src/components/Classifier/Classifier.spec.js | 12 ++++++++++-- .../Classifier/Classifier.workflows.spec.js | 6 +++++- .../Classifier/ClassifierContainer.spec.js | 6 +++++- .../MultiFrameViewerContainer.spec.js | 6 +++++- .../SingleImageViewerContainer.spec.js | 6 +++++- 5 files changed, 30 insertions(+), 6 deletions(-) diff --git a/packages/lib-classifier/src/components/Classifier/Classifier.spec.js b/packages/lib-classifier/src/components/Classifier/Classifier.spec.js index 4b5504c810..392f1e320f 100644 --- a/packages/lib-classifier/src/components/Classifier/Classifier.spec.js +++ b/packages/lib-classifier/src/components/Classifier/Classifier.spec.js @@ -38,7 +38,11 @@ describe('Components > Classifier', function () { constructor () { this.naturalHeight = 1000 this.naturalWidth = 500 - setTimeout(() => this.onload(), 500) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), 500) } } @@ -46,7 +50,11 @@ describe('Components > Classifier', function () { constructor () { this.naturalHeight = 1000 this.naturalWidth = 500 - setTimeout(() => this.onload(), 5000) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), 5000) } } diff --git a/packages/lib-classifier/src/components/Classifier/Classifier.workflows.spec.js b/packages/lib-classifier/src/components/Classifier/Classifier.workflows.spec.js index b0fb2c9793..864dc0755f 100644 --- a/packages/lib-classifier/src/components/Classifier/Classifier.workflows.spec.js +++ b/packages/lib-classifier/src/components/Classifier/Classifier.workflows.spec.js @@ -76,7 +76,11 @@ function testWorkflow(workflowSnapshot, workflowStrings) { constructor () { this.naturalHeight = 1000 this.naturalWidth = 500 - setTimeout(() => this.onload(), 500) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), 500) } }) diff --git a/packages/lib-classifier/src/components/Classifier/ClassifierContainer.spec.js b/packages/lib-classifier/src/components/Classifier/ClassifierContainer.spec.js index 70580e40a9..b5e4ed9e69 100644 --- a/packages/lib-classifier/src/components/Classifier/ClassifierContainer.spec.js +++ b/packages/lib-classifier/src/components/Classifier/ClassifierContainer.spec.js @@ -33,8 +33,12 @@ describe('components > ClassifierContainer', function () { constructor () { this.naturalHeight = 1000 this.naturalWidth = 500 + const fakeLoadEvent = { + ...new Event('load'), + target: this + } setTimeout(() => { - this.onload() + this.onload(fakeLoadEvent) }, 500) } } diff --git a/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/MultiFrameViewer/MultiFrameViewerContainer.spec.js b/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/MultiFrameViewer/MultiFrameViewerContainer.spec.js index 225b0edfae..8845391cfc 100644 --- a/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/MultiFrameViewer/MultiFrameViewerContainer.spec.js +++ b/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/MultiFrameViewer/MultiFrameViewerContainer.spec.js @@ -26,7 +26,11 @@ describe('Component > MultiFrameViewerContainer', function () { constructor () { this.naturalHeight = height this.naturalWidth = width - setTimeout(() => this.onload(), DELAY) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), DELAY) } } diff --git a/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewerContainer.spec.js b/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewerContainer.spec.js index ff11876cc0..ef12e77822 100644 --- a/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewerContainer.spec.js +++ b/packages/lib-classifier/src/components/Classifier/components/SubjectViewer/components/SingleImageViewer/SingleImageViewerContainer.spec.js @@ -24,7 +24,11 @@ describe('Component > SingleImageViewerContainer', function () { constructor () { this.naturalHeight = height this.naturalWidth = width - setTimeout(() => this.onload(), DELAY) + const fakeLoadEvent = { + ...new Event('load'), + target: this + } + setTimeout(() => this.onload(fakeLoadEvent), DELAY) } }