From 15d0927f531a49d2744fd19638256a5712a9c22b Mon Sep 17 00:00:00 2001 From: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Date: Wed, 10 May 2023 10:50:38 +0300 Subject: [PATCH] Site Editor: Improve loading experience (v3) (#50222) * Data: Introduce hasResolvingSelectors metadata selector * Edit Site: Display loading screen if we have resolving requests * Move loading logic to a hook * Inline again, loading spinner only in beginning, fix flickering * Hide canvas-overflowing content * Fix resolveSelect tests * e2e: try waiting for spinner to disappear * Add some additional safety to selector * Simplify timeout clearing * Add a comment on the arbitrary timeout --- .../src/redux-store/metadata/selectors.js | 15 +++++++ .../redux-store/metadata/test/selectors.js | 32 +++++++++++++++ packages/data/src/redux-store/test/index.js | 1 + packages/e2e-test-utils/src/site-editor.js | 2 + .../edit-site/src/components/editor/index.js | 39 ++++++++++++++++--- .../src/components/layout/style.scss | 1 + 6 files changed, 85 insertions(+), 5 deletions(-) diff --git a/packages/data/src/redux-store/metadata/selectors.js b/packages/data/src/redux-store/metadata/selectors.js index 329074cd0a83b7..4e5a999567302d 100644 --- a/packages/data/src/redux-store/metadata/selectors.js +++ b/packages/data/src/redux-store/metadata/selectors.js @@ -131,3 +131,18 @@ export function isResolving( state, selectorName, args ) { export function getCachedResolvers( state ) { return state; } + +/** + * Whether the store has any currently resolving selectors. + * + * @param {State} state Data state. + * + * @return {boolean} True if one or more selectors are resolving, false otherwise. + */ +export function hasResolvingSelectors( state ) { + return [ ...Object.values( state ) ].some( ( selectorState ) => + [ ...selectorState._map.values() ].some( + ( resolution ) => resolution[ 1 ]?.status === 'resolving' + ) + ); +} diff --git a/packages/data/src/redux-store/metadata/test/selectors.js b/packages/data/src/redux-store/metadata/test/selectors.js index d84ef52bddd048..2eea14a7b6059f 100644 --- a/packages/data/src/redux-store/metadata/test/selectors.js +++ b/packages/data/src/redux-store/metadata/test/selectors.js @@ -324,3 +324,35 @@ describe( 'getResolutionError', () => { ).toBeFalsy(); } ); } ); + +describe( 'hasResolvingSelectors', () => { + let registry; + beforeEach( () => { + registry = createRegistry(); + registry.registerStore( 'testStore', testStore ); + } ); + + it( 'returns false if no requests have started', () => { + const { hasResolvingSelectors } = registry.select( 'testStore' ); + const result = hasResolvingSelectors(); + + expect( result ).toBe( false ); + } ); + + it( 'returns false if all requests have finished', () => { + registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] ); + registry.dispatch( 'testStore' ).finishResolution( 'getFoo', [] ); + const { hasResolvingSelectors } = registry.select( 'testStore' ); + const result = hasResolvingSelectors(); + + expect( result ).toBe( false ); + } ); + + it( 'returns true if has started but not finished', () => { + registry.dispatch( 'testStore' ).startResolution( 'getFoo', [] ); + const { hasResolvingSelectors } = registry.select( 'testStore' ); + const result = hasResolvingSelectors(); + + expect( result ).toBe( true ); + } ); +} ); diff --git a/packages/data/src/redux-store/test/index.js b/packages/data/src/redux-store/test/index.js index daca128480daeb..e3cc2c727dbc5f 100644 --- a/packages/data/src/redux-store/test/index.js +++ b/packages/data/src/redux-store/test/index.js @@ -281,6 +281,7 @@ describe( 'resolveSelect', () => { it( 'returns only store native selectors and excludes all meta ones', () => { expect( Object.keys( registry.resolveSelect( 'store' ) ) ).toEqual( [ + 'hasResolvingSelectors', 'getItems', 'getItemsNoResolver', ] ); diff --git a/packages/e2e-test-utils/src/site-editor.js b/packages/e2e-test-utils/src/site-editor.js index 97f1c0e16bdda6..619fc8b9cf630b 100644 --- a/packages/e2e-test-utils/src/site-editor.js +++ b/packages/e2e-test-utils/src/site-editor.js @@ -10,6 +10,7 @@ import { addQueryArgs } from '@wordpress/url'; const SELECTORS = { visualEditor: '.edit-site-visual-editor iframe', + loadingSpinner: '.edit-site-canvas-spinner', }; /** @@ -128,6 +129,7 @@ export async function visitSiteEditor( query, skipWelcomeGuide = true ) { await visitAdminPage( 'site-editor.php', query ); await page.waitForSelector( SELECTORS.visualEditor ); + await page.waitForSelector( SELECTORS.loadingSpinner, { hidden: true } ); if ( skipWelcomeGuide ) { await disableSiteEditorWelcomeGuide(); diff --git a/packages/edit-site/src/components/editor/index.js b/packages/edit-site/src/components/editor/index.js index eb05e2526a55f9..26a710fc03b4e7 100644 --- a/packages/edit-site/src/components/editor/index.js +++ b/packages/edit-site/src/components/editor/index.js @@ -1,10 +1,10 @@ /** * WordPress dependencies */ -import { useMemo } from '@wordpress/element'; +import { useEffect, useMemo, useRef, useState } from '@wordpress/element'; import { useSelect, useDispatch } from '@wordpress/data'; import { Notice } from '@wordpress/components'; -import { EntityProvider } from '@wordpress/core-data'; +import { EntityProvider, store as coreStore } from '@wordpress/core-data'; import { store as preferencesStore } from '@wordpress/preferences'; import { BlockContextProvider, @@ -153,12 +153,41 @@ export default function Editor() { // action in from double-announcing. useTitle( hasLoadedPost && title ); - if ( ! hasLoadedPost ) { - return ; - } + const { hasResolvingSelectors } = useSelect( ( select ) => { + return { + hasResolvingSelectors: select( coreStore ).hasResolvingSelectors(), + }; + } ); + const [ loaded, setLoaded ] = useState( false ); + const timeoutRef = useRef( null ); + + useEffect( () => { + if ( ! hasResolvingSelectors && ! loaded ) { + clearTimeout( timeoutRef.current ); + + /* + * We're using an arbitrary 1s timeout here to catch brief moments + * without any resolving selectors that would result in displaying + * brief flickers of loading state and loaded state. + * + * It's worth experimenting with different values, since this also + * adds 1s of artificial delay after loading has finished. + */ + timeoutRef.current = setTimeout( () => { + setLoaded( true ); + }, 1000 ); + + return () => { + clearTimeout( timeoutRef.current ); + }; + } + }, [ loaded, hasResolvingSelectors ] ); + + const isLoading = ! loaded || ! hasLoadedPost; return ( <> + { isLoading ? : null } { isEditMode && } div { color: $gray-900;