From 37454b7b9f97e1e1080f039293cbf9b3ce68b464 Mon Sep 17 00:00:00 2001 From: Jean-Louis Leysens Date: Tue, 25 Oct 2022 14:15:13 +0200 Subject: [PATCH] [Files] Fix image intersection observer behavior (#143843) * react with loading spinner to new page or retry requests * do not fetch on every change? in files example app * remove lazy from the file picker image * absolute position the image tag to ensure that it is visible, this ensures the intersection observer will fire * remove the "lazy" prop from the image component * remove unnecessary if * added comment and increase page size of file picker in files example plugin * make the empty "hidden" image occupy the full container space --- .../files_example/public/components/app.tsx | 6 ++-- .../public/components/file_picker.tsx | 2 +- .../file_picker/components/file_card.tsx | 5 --- .../file_picker/file_picker_state.ts | 13 +++++--- .../components/image/components/img.tsx | 10 ++++-- .../files/public/components/image/image.tsx | 31 ++----------------- 6 files changed, 23 insertions(+), 44 deletions(-) diff --git a/x-pack/examples/files_example/public/components/app.tsx b/x-pack/examples/files_example/public/components/app.tsx index d3dfbdeb71874..afdf8be1f4f6e 100644 --- a/x-pack/examples/files_example/public/components/app.tsx +++ b/x-pack/examples/files_example/public/components/app.tsx @@ -36,8 +36,10 @@ interface FilesExampleAppDeps { type ListResponse = FilesClientResponses['list']; export const FilesExampleApp = ({ files, notifications }: FilesExampleAppDeps) => { - const { data, isLoading, error, refetch } = useQuery(['files'], () => - files.example.list() + const { data, isLoading, error, refetch } = useQuery( + ['files'], + () => files.example.list(), + { refetchOnWindowFocus: false } ); const [showUploadModal, setShowUploadModal] = useState(false); const [showFilePickerModal, setShowFilePickerModal] = useState(false); diff --git a/x-pack/examples/files_example/public/components/file_picker.tsx b/x-pack/examples/files_example/public/components/file_picker.tsx index 2bf5530655ba3..3c2178b299ea2 100644 --- a/x-pack/examples/files_example/public/components/file_picker.tsx +++ b/x-pack/examples/files_example/public/components/file_picker.tsx @@ -18,5 +18,5 @@ interface Props { } export const MyFilePicker: FunctionComponent = ({ onClose, onDone }) => { - return ; + return ; }; diff --git a/x-pack/plugins/files/public/components/file_picker/components/file_card.tsx b/x-pack/plugins/files/public/components/file_picker/components/file_card.tsx index 4c290b1b114e7..88c77f36a6c00 100644 --- a/x-pack/plugins/files/public/components/file_picker/components/file_card.tsx +++ b/x-pack/plugins/files/public/components/file_picker/components/file_card.tsx @@ -58,11 +58,6 @@ export const FileCard: FunctionComponent = ({ file }) => { `} meta={file.meta as FileImageMetadata} src={client.getDownloadHref({ id: file.id, fileKind: kind })} - // There is an issue where the intersection observer does not fire reliably. - // I'm not sure if this is becuause of the image being in a modal - // The result is that the image does not always get loaded. - // TODO: Investigate this behaviour further - lazy={false} /> ) : (
this.setIsLoading(true))).subscribe(), this.internalIsLoading$ .pipe(debounceTime(100), distinctUntilChanged()) .subscribe(this.isLoading$), ]; } + private readonly requests$ = combineLatest([ + this.currentPage$.pipe(distinctUntilChanged()), + this.query$.pipe(distinctUntilChanged(), debounceTime(100)), + this.retry$, + ]); + /** * File objects we have loaded on the front end, stored here so that it can * easily be passed to all relevant UI. @@ -74,11 +81,7 @@ export class FilePickerState { * @note This is not explicitly kept in sync with the selected files! * @note This is not explicitly kept in sync with the selected files! */ - public readonly files$ = combineLatest([ - this.currentPage$.pipe(distinctUntilChanged()), - this.query$.pipe(distinctUntilChanged(), debounceTime(100)), - this.retry$, - ]).pipe( + public readonly files$ = this.requests$.pipe( switchMap(([page, query]) => this.sendRequest(page, query)), tap(({ total }) => this.updateTotalPages({ total })), tap(({ total }) => this.hasFiles$.next(Boolean(total))), diff --git a/x-pack/plugins/files/public/components/image/components/img.tsx b/x-pack/plugins/files/public/components/image/components/img.tsx index 295b062ca1fd8..953eb93a19917 100644 --- a/x-pack/plugins/files/public/components/image/components/img.tsx +++ b/x-pack/plugins/files/public/components/image/components/img.tsx @@ -12,21 +12,25 @@ import { css } from '@emotion/react'; import { sizes } from '../styles'; export interface Props extends ImgHTMLAttributes { - hidden: boolean; size?: EuiImageSize; observerRef: (el: null | HTMLImageElement) => void; } export const Img = React.forwardRef( - ({ observerRef, src, hidden, size, ...rest }, ref) => { + ({ observerRef, src, size, ...rest }, ref) => { const { euiTheme } = useEuiTheme(); const styles = [ css` transition: opacity ${euiTheme.animation.extraFast}; `, - hidden + !src ? css` visibility: hidden; + position: absolute; // ensure that empty img tag occupies full container + top: 0; + right: 0; + bottom: 0; + left: 0; ` : undefined, size ? sizes[size] : undefined, diff --git a/x-pack/plugins/files/public/components/image/image.tsx b/x-pack/plugins/files/public/components/image/image.tsx index b83739d180c94..e353fced3ec3e 100644 --- a/x-pack/plugins/files/public/components/image/image.tsx +++ b/x-pack/plugins/files/public/components/image/image.tsx @@ -32,15 +32,6 @@ export interface Props extends ImgHTMLAttributes { * Emits when the image first becomes visible */ onFirstVisible?: () => void; - - /** - * As an optimisation images are only loaded when they are visible. - * This setting overrides this behavior and loads an image as soon as the - * component mounts. - * - * @default true - */ - lazy?: boolean; } /** @@ -55,33 +46,18 @@ export interface Props extends ImgHTMLAttributes { */ export const Image = React.forwardRef( ( - { - src, - alt, - onFirstVisible, - onLoad, - onError, - meta, - wrapperProps, - size = 'original', - lazy = true, - ...rest - }, + { src, alt, onFirstVisible, onLoad, onError, meta, wrapperProps, size = 'original', ...rest }, ref ) => { const [isLoaded, setIsLoaded] = useState(false); const [blurDelayExpired, setBlurDelayExpired] = useState(false); const { isVisible, ref: observerRef } = useViewportObserver({ onFirstVisible }); - const loadImage = lazy ? isVisible : true; - useEffect(() => { - let unmounted = false; const id = window.setTimeout(() => { - if (!unmounted) setBlurDelayExpired(true); + setBlurDelayExpired(true); }, 200); return () => { - unmounted = true; window.clearTimeout(id); }; }, []); @@ -112,8 +88,7 @@ export const Image = React.forwardRef( observerRef={observerRef} ref={ref} size={size} - hidden={!loadImage} - src={loadImage ? src : undefined} + src={isVisible ? src : undefined} alt={alt} onLoad={(ev) => { setIsLoaded(true);