Repository navigation
Font library: Add skeleton loader for the font preview images #81047
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
Changes from 4 commits
aa51d44
60fa64b
a31bd10
acba2f6
ab7780f
1b01cab
b8ca6ff
5f56ebd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,13 +1,18 @@ | ||
| import { __experimentalText as WCText } from '@wordpress/components'; | ||
| import clsx from 'clsx'; | ||
| import { useContext, useEffect, useState, useRef } from '@wordpress/element'; | ||
| import type { FontFamily, FontFace } from '@wordpress/core-data'; | ||
| import { Text, Skeleton } from '@wordpress/ui'; | ||
| import { FontLibraryContext } from './context'; | ||
| import { | ||
| getFacePreviewStyle, | ||
| getFamilyPreviewStyle, | ||
| } from './utils/preview-styles'; | ||
| import type { FontDemoProps } from './types'; | ||
|
|
||
| // Previews already loaded this session, so reopening the library doesn't flash | ||
| // a placeholder for images the browser already has. | ||
| const loadedPreviews = new Set< string >(); | ||
|
|
||
| function getPreviewUrl( fontFace: FontFace ): string | undefined { | ||
| if ( fontFace.preview ) { | ||
| return fontFace.preview; | ||
|
|
@@ -47,19 +52,29 @@ function getDisplayFontFace( font: FontFamily | FontFace ): FontFace { | |
|
|
||
| function FontDemo( { font, text }: FontDemoProps ) { | ||
| const ref = useRef< HTMLDivElement >( null ); | ||
| const imgRef = useRef< HTMLImageElement >( null ); | ||
|
|
||
| const fontFace = getDisplayFontFace( font ); | ||
| const style = getFamilyPreviewStyle( font ); | ||
| text = text || ( 'name' in font ? font.name : '' ); | ||
| const customPreviewUrl = font.preview; | ||
|
|
||
| const previewUrl = customPreviewUrl ?? getPreviewUrl( fontFace ); | ||
| const isPreviewImage = Boolean( | ||
| previewUrl && /\.(png|jpg|jpeg|gif|svg)$/i.test( previewUrl ) | ||
| ); | ||
|
|
||
| const [ isIntersecting, setIsIntersecting ] = useState< boolean >( false ); | ||
| const [ isAssetLoaded, setIsAssetLoaded ] = useState< boolean >( false ); | ||
| const [ isAssetLoaded, setIsAssetLoaded ] = useState< boolean >( | ||
| () => !! previewUrl && loadedPreviews.has( previewUrl ) | ||
| ); | ||
| const { loadFontFaceAsset } = useContext( FontLibraryContext ); | ||
|
|
||
| const previewUrl = customPreviewUrl ?? getPreviewUrl( fontFace ); | ||
| const isPreviewImage = | ||
| previewUrl && previewUrl.match( /\.(png|jpg|jpeg|gif|svg)$/i ); | ||
| // The previews scale with the label, so estimate ~12px per character. | ||
| const estimatedImageWidth = Math.min( | ||
| Math.max( text.length * 12, 48 ), | ||
| 300 | ||
| ); | ||
|
|
||
| const faceStyles = getFacePreviewStyle( fontFace ); | ||
| const textDemoStyle = { | ||
|
|
@@ -71,19 +86,33 @@ function FontDemo( { font, text }: FontDemoProps ) { | |
| }; | ||
|
|
||
| useEffect( () => { | ||
| // Only font files need the viewport check. Image previews are lazy | ||
| // loaded by the browser, and every row in the collection has one. | ||
| if ( isPreviewImage ) { | ||
| return; | ||
| } | ||
| const observer = new window.IntersectionObserver( ( [ entry ] ) => { | ||
| setIsIntersecting( entry.isIntersecting ); | ||
| }, {} ); | ||
| if ( ref.current ) { | ||
| observer.observe( ref.current ); | ||
| } | ||
| return () => observer.disconnect(); | ||
| }, [ ref ] ); | ||
| }, [ isPreviewImage ] ); | ||
|
|
||
| // Re-check on preview change, since an already complete image won't fire | ||
| // onLoad and the component is reused across fonts. | ||
| useEffect( () => { | ||
| setIsAssetLoaded( | ||
| ( !! previewUrl && loadedPreviews.has( previewUrl ) ) || | ||
| !! imgRef.current?.complete | ||
| ); | ||
| }, [ previewUrl ] ); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this current state management,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point! Actually while checking this, I ended up seeing that it could be simpler than what I initially proposed. If we track which url was loaded we won't have the extra render. That way we could also get rid of the effect and the ref. Updated 1b01cab |
||
|
|
||
| useEffect( () => { | ||
| const loadAsset = async () => { | ||
| if ( isIntersecting ) { | ||
| if ( ! isPreviewImage && fontFace.src ) { | ||
| if ( isIntersecting && ! isPreviewImage ) { | ||
| if ( fontFace.src ) { | ||
| await loadFontFaceAsset( fontFace ); | ||
| } | ||
| setIsAssetLoaded( true ); | ||
|
|
@@ -93,21 +122,41 @@ function FontDemo( { font, text }: FontDemoProps ) { | |
| }, [ fontFace, isIntersecting, loadFontFaceAsset, isPreviewImage ] ); | ||
|
|
||
| return ( | ||
| <div ref={ ref }> | ||
| <div ref={ ref } className="font-library__font-demo"> | ||
| { isPreviewImage ? ( | ||
| <img | ||
| src={ previewUrl } | ||
| loading="lazy" | ||
| alt={ text } | ||
| className="font-library__font-variant_demo-image" | ||
| /> | ||
| <> | ||
| { ! isAssetLoaded && ( | ||
| <Skeleton | ||
| className="font-library__font-variant-demo-skeleton" | ||
| style={ { width: estimatedImageWidth } } | ||
| /> | ||
| ) } | ||
| <img | ||
| ref={ imgRef } | ||
| src={ previewUrl } | ||
| loading="lazy" | ||
| alt={ text } | ||
| onLoad={ () => { | ||
| if ( previewUrl ) { | ||
| loadedPreviews.add( previewUrl ); | ||
| } | ||
| setIsAssetLoaded( true ); | ||
| } } | ||
| // Also on failure, otherwise skeleton will pulse forever. | ||
| onError={ () => setIsAssetLoaded( true ) } | ||
| className={ clsx( | ||
| 'font-library__font-variant_demo-image', | ||
| { 'is-loading': ! isAssetLoaded } | ||
| ) } | ||
| /> | ||
| </> | ||
| ) : ( | ||
| <WCText | ||
| <Text | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not a blocker, but unclear to me whether a
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, not really necessary Although I found a bug with variable fonts while trying to migrate. They declare weights as ranges, and we pass that straight to the If we move off
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in #81748 |
||
| style={ textDemoStyle } | ||
| className="font-library__font-variant_demo-text" | ||
| > | ||
| { text } | ||
| </WCText> | ||
| </Text> | ||
| ) } | ||
| </div> | ||
| ); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs to be repositioned into the Unreleased section before merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, thanks as always! 🙏