From ee588f14d6d3d5c8f69bba307a8cfad64584daae Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Tue, 6 Feb 2024 15:05:35 +1100 Subject: [PATCH 1/5] Site Editor: Only show 'Back' button when came from an editor canvas --- .../block-editor/use-site-editor-settings.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js index 77e9e69f02a984..40fab6b9d8021c 100644 --- a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js +++ b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js @@ -6,6 +6,7 @@ import { useMemo } from '@wordpress/element'; import { store as coreStore } from '@wordpress/core-data'; import { privateApis as editorPrivateApis } from '@wordpress/editor'; import { privateApis as routerPrivateApis } from '@wordpress/router'; +import { usePrevious } from '@wordpress/compose'; /** * Internal dependencies @@ -92,13 +93,18 @@ function useArchiveLabel( templateSlug ) { function useGoBack() { const location = useLocation(); + const previousLocation = usePrevious( location ); const history = useHistory(); const goBack = useMemo( () => { const isFocusMode = location.params.focusMode || - FOCUSABLE_ENTITIES.includes( location.params.postType ); - return isFocusMode ? () => history.back() : undefined; - }, [ location.params.focusMode, location.params.postType, history ] ); + ( location.params.postId && + FOCUSABLE_ENTITIES.includes( location.params.postType ) ); + const didComeFromEditorCanvas = + previousLocation?.params.canvas === 'edit'; + const showBackButton = isFocusMode && didComeFromEditorCanvas; + return showBackButton ? () => history.back() : undefined; + }, [ location ] ); return goBack; } From aef38f8d31ad3d9bebdb916eb3c9204d589a0eb4 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 7 Feb 2024 12:46:31 +1100 Subject: [PATCH 2/5] Check for presense of postType and postId --- .../src/components/block-editor/use-site-editor-settings.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js index 40fab6b9d8021c..99077fe6d9c101 100644 --- a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js +++ b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js @@ -101,6 +101,8 @@ function useGoBack() { ( location.params.postId && FOCUSABLE_ENTITIES.includes( location.params.postType ) ); const didComeFromEditorCanvas = + previousLocation?.params.postId && + previousLocation?.params.postType && previousLocation?.params.canvas === 'edit'; const showBackButton = isFocusMode && didComeFromEditorCanvas; return showBackButton ? () => history.back() : undefined; From 8068fe31a679ab98323df11ec987392028ce59aa Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 7 Feb 2024 15:06:30 +1100 Subject: [PATCH 3/5] usePrevious: Update implementation to return previous value, instead of value from previous render See https://www.developerway.com/posts/implementing-advanced-use-previous-hook for a good explanation of the problem. --- packages/compose/README.md | 6 +++-- .../compose/src/hooks/use-previous/index.ts | 26 +++++++++---------- .../src/hooks/use-previous/test/index.js | 23 ++++++++++++++++ .../block-editor/use-site-editor-settings.js | 2 +- 4 files changed, 41 insertions(+), 16 deletions(-) create mode 100644 packages/compose/src/hooks/use-previous/test/index.js diff --git a/packages/compose/README.md b/packages/compose/README.md index 7eb70a7300f07f..2fdc76dec6ce97 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -444,7 +444,9 @@ _Returns_ ### usePrevious -Use something's value from the previous render. Based on . +Use the previous value of a prop or state in a component. + +Based on , but uses refs to avoid rendering twice. See . _Parameters_ @@ -452,7 +454,7 @@ _Parameters_ _Returns_ -- `T | undefined`: The value from the previous render. +- `T | undefined`: What the value was before it was updated. ### useReducedMotion diff --git a/packages/compose/src/hooks/use-previous/index.ts b/packages/compose/src/hooks/use-previous/index.ts index 673495320c5040..fa4739b9d5e901 100644 --- a/packages/compose/src/hooks/use-previous/index.ts +++ b/packages/compose/src/hooks/use-previous/index.ts @@ -1,24 +1,24 @@ /** * WordPress dependencies */ -import { useEffect, useRef } from '@wordpress/element'; +import { useRef } from '@wordpress/element'; /** - * Use something's value from the previous render. - * Based on https://usehooks.com/usePrevious/. + * Use the previous value of a prop or state in a component. + * + * Based on https://usehooks.com/usePrevious/, but uses refs to avoid rendering twice. See + * https://www.developerway.com/posts/implementing-advanced-use-previous-hook. * * @param value The value to track. * - * @return The value from the previous render. + * @return What the value was before it was updated. */ export default function usePrevious< T >( value: T ): T | undefined { - const ref = useRef< T >(); - - // Store current value in ref. - useEffect( () => { - ref.current = value; - }, [ value ] ); // Re-run when value changes. - - // Return previous value (happens before update in useEffect above). - return ref.current; + const valueRef = useRef< T >( value ); + const previousRef = useRef< T | undefined >(); + if ( value !== valueRef.current ) { + previousRef.current = valueRef.current; + valueRef.current = value; + } + return previousRef.current; } diff --git a/packages/compose/src/hooks/use-previous/test/index.js b/packages/compose/src/hooks/use-previous/test/index.js new file mode 100644 index 00000000000000..c07d61e2692afb --- /dev/null +++ b/packages/compose/src/hooks/use-previous/test/index.js @@ -0,0 +1,23 @@ +/** + * External dependencies + */ +import { renderHook } from '@testing-library/react'; + +/** + * Internal dependencies + */ +import usePrevious from '..'; + +describe( 'usePrevious', () => { + it( 'should return the previous value', () => { + const { result, rerender } = renderHook( + ( { value } ) => usePrevious( value ), + { initialProps: { value: 1 } } + ); + expect( result.current ).toBeUndefined(); + rerender( { value: 2 } ); + expect( result.current ).toBe( 1 ); + rerender( { value: 3 } ); + expect( result.current ).toBe( 2 ); + } ); +} ); diff --git a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js index 99077fe6d9c101..112c171c5462a0 100644 --- a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js +++ b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js @@ -106,7 +106,7 @@ function useGoBack() { previousLocation?.params.canvas === 'edit'; const showBackButton = isFocusMode && didComeFromEditorCanvas; return showBackButton ? () => history.back() : undefined; - }, [ location ] ); + }, [ location, previousLocation, history ] ); return goBack; } From 26ebd904c5f10825fcef9eee0a7498d59c5932a0 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 7 Feb 2024 15:22:53 +1100 Subject: [PATCH 4/5] Revert "usePrevious: Update implementation to return previous value, instead of value from previous render" This reverts commit 8068fe31a679ab98323df11ec987392028ce59aa. --- packages/compose/README.md | 6 ++--- .../compose/src/hooks/use-previous/index.ts | 26 +++++++++---------- .../src/hooks/use-previous/test/index.js | 23 ---------------- .../block-editor/use-site-editor-settings.js | 2 +- 4 files changed, 16 insertions(+), 41 deletions(-) delete mode 100644 packages/compose/src/hooks/use-previous/test/index.js diff --git a/packages/compose/README.md b/packages/compose/README.md index 2fdc76dec6ce97..7eb70a7300f07f 100644 --- a/packages/compose/README.md +++ b/packages/compose/README.md @@ -444,9 +444,7 @@ _Returns_ ### usePrevious -Use the previous value of a prop or state in a component. - -Based on , but uses refs to avoid rendering twice. See . +Use something's value from the previous render. Based on . _Parameters_ @@ -454,7 +452,7 @@ _Parameters_ _Returns_ -- `T | undefined`: What the value was before it was updated. +- `T | undefined`: The value from the previous render. ### useReducedMotion diff --git a/packages/compose/src/hooks/use-previous/index.ts b/packages/compose/src/hooks/use-previous/index.ts index fa4739b9d5e901..673495320c5040 100644 --- a/packages/compose/src/hooks/use-previous/index.ts +++ b/packages/compose/src/hooks/use-previous/index.ts @@ -1,24 +1,24 @@ /** * WordPress dependencies */ -import { useRef } from '@wordpress/element'; +import { useEffect, useRef } from '@wordpress/element'; /** - * Use the previous value of a prop or state in a component. - * - * Based on https://usehooks.com/usePrevious/, but uses refs to avoid rendering twice. See - * https://www.developerway.com/posts/implementing-advanced-use-previous-hook. + * Use something's value from the previous render. + * Based on https://usehooks.com/usePrevious/. * * @param value The value to track. * - * @return What the value was before it was updated. + * @return The value from the previous render. */ export default function usePrevious< T >( value: T ): T | undefined { - const valueRef = useRef< T >( value ); - const previousRef = useRef< T | undefined >(); - if ( value !== valueRef.current ) { - previousRef.current = valueRef.current; - valueRef.current = value; - } - return previousRef.current; + const ref = useRef< T >(); + + // Store current value in ref. + useEffect( () => { + ref.current = value; + }, [ value ] ); // Re-run when value changes. + + // Return previous value (happens before update in useEffect above). + return ref.current; } diff --git a/packages/compose/src/hooks/use-previous/test/index.js b/packages/compose/src/hooks/use-previous/test/index.js deleted file mode 100644 index c07d61e2692afb..00000000000000 --- a/packages/compose/src/hooks/use-previous/test/index.js +++ /dev/null @@ -1,23 +0,0 @@ -/** - * External dependencies - */ -import { renderHook } from '@testing-library/react'; - -/** - * Internal dependencies - */ -import usePrevious from '..'; - -describe( 'usePrevious', () => { - it( 'should return the previous value', () => { - const { result, rerender } = renderHook( - ( { value } ) => usePrevious( value ), - { initialProps: { value: 1 } } - ); - expect( result.current ).toBeUndefined(); - rerender( { value: 2 } ); - expect( result.current ).toBe( 1 ); - rerender( { value: 3 } ); - expect( result.current ).toBe( 2 ); - } ); -} ); diff --git a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js index 112c171c5462a0..99077fe6d9c101 100644 --- a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js +++ b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js @@ -106,7 +106,7 @@ function useGoBack() { previousLocation?.params.canvas === 'edit'; const showBackButton = isFocusMode && didComeFromEditorCanvas; return showBackButton ? () => history.back() : undefined; - }, [ location, previousLocation, history ] ); + }, [ location ] ); return goBack; } From 2a012faede8e2492a5eba68e05d335e391f211cd Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Wed, 7 Feb 2024 15:25:12 +1100 Subject: [PATCH 5/5] Ignore react-hooks/exhaustive-deps for now --- .../src/components/block-editor/use-site-editor-settings.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js index 99077fe6d9c101..a115094bc1e510 100644 --- a/packages/edit-site/src/components/block-editor/use-site-editor-settings.js +++ b/packages/edit-site/src/components/block-editor/use-site-editor-settings.js @@ -106,7 +106,11 @@ function useGoBack() { previousLocation?.params.canvas === 'edit'; const showBackButton = isFocusMode && didComeFromEditorCanvas; return showBackButton ? () => history.back() : undefined; - }, [ location ] ); + // Disable reason: previousLocation changes when the component updates for any reason, not + // just when location changes. Until this is fixed we can't add it to deps. See + // https://github.com/WordPress/gutenberg/pull/58710#discussion_r1479219465. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [ location, history ] ); return goBack; }