From 09b48d68adea17d21cad20f4ab3056e32e24109e Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sun, 25 Aug 2024 12:25:05 +0200 Subject: [PATCH 1/5] Fix isInitial logic --- .../navigator/navigator-provider/component.tsx | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 8e596778c7f21c..eb577046889023 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -155,20 +155,22 @@ function routerReducer( break; case 'goto': const goToNewState = goTo( state, action.path, action.options ); - currentLocation = goToNewState.currentLocation; + currentLocation = { + ...goToNewState.currentLocation, + isInitial: false, + }; focusSelectors = goToNewState.focusSelectors; break; case 'gotoparent': const goToParentNewState = goToParent( state, action.options ); - currentLocation = goToParentNewState.currentLocation; + currentLocation = { + ...goToParentNewState.currentLocation, + isInitial: false, + }; focusSelectors = goToParentNewState.focusSelectors; break; } - if ( currentLocation?.path === state.initialPath ) { - currentLocation = { ...currentLocation, isInitial: true }; - } - // Return early in case there is no change if ( screens === state.screens && @@ -220,7 +222,7 @@ function UnconnectedNavigatorProvider( initialPathProp, ( path ) => ( { screens: [], - currentLocation: { path }, + currentLocation: { path, isInitial: true }, matchedPath: undefined, focusSelectors: new Map(), initialPath: initialPathProp, From bfaccad8339fea235ed188b2d23d4ff9fadc8aa5 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sun, 25 Aug 2024 12:25:20 +0200 Subject: [PATCH 2/5] Use '@wordpress/warning' instead of console.warn --- .../components/src/navigator/navigator-provider/component.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index eb577046889023..de9e1ede7500ff 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -8,6 +8,7 @@ import type { ForwardedRef } from 'react'; */ import { useMemo, useReducer } from '@wordpress/element'; import isShallowEqual from '@wordpress/is-shallow-equal'; +import warning from '@wordpress/warning'; /** * Internal dependencies @@ -46,8 +47,7 @@ type RouterState = { function addScreen( { screens }: RouterState, screen: Screen ) { if ( screens.some( ( s ) => s.path === screen.path ) ) { - // eslint-disable-next-line no-console - console.warn( + warning( `Navigator: a screen with path ${ screen.path } already exists. The screen with id ${ screen.id } will not be added.` ); From 3e6dcbd0391be97e3b8308c77fd3fc19513f4994 Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sun, 25 Aug 2024 12:33:04 +0200 Subject: [PATCH 3/5] Improved destructuring and isInitial assignment --- .../navigator-provider/component.tsx | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index de9e1ede7500ff..0e3c3b9a5fc629 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -65,7 +65,8 @@ function goTo( path: string, options: NavigateOptions = {} ) { - const { currentLocation, focusSelectors } = state; + const { focusSelectors } = state; + const currentLocation = { ...state.currentLocation, isInitial: false }; const { // Default assignments @@ -120,7 +121,8 @@ function goToParent( state: RouterState, options: NavigateToParentOptions = {} ) { - const { currentLocation, screens, focusSelectors } = state; + const { screens, focusSelectors } = state; + const currentLocation = { ...state.currentLocation, isInitial: false }; const currentPath = currentLocation?.path; if ( currentPath === undefined ) { return { currentLocation, focusSelectors }; @@ -154,20 +156,17 @@ function routerReducer( screens = removeScreen( state, action.screen ); break; case 'goto': - const goToNewState = goTo( state, action.path, action.options ); - currentLocation = { - ...goToNewState.currentLocation, - isInitial: false, - }; - focusSelectors = goToNewState.focusSelectors; + ( { currentLocation, focusSelectors } = goTo( + state, + action.path, + action.options + ) ); break; case 'gotoparent': - const goToParentNewState = goToParent( state, action.options ); - currentLocation = { - ...goToParentNewState.currentLocation, - isInitial: false, - }; - focusSelectors = goToParentNewState.focusSelectors; + ( { currentLocation, focusSelectors } = goToParent( + state, + action.options + ) ); break; } From fe16bbf0bbe6f9f4c8cdccdc452005cab805c8da Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Sun, 25 Aug 2024 14:09:13 +0200 Subject: [PATCH 4/5] Refactor focus selector logic, better cleanup, lazier map copy --- .../navigator-provider/component.tsx | 27 ++++++++++++------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 0e3c3b9a5fc629..101ab57ff6ea4d 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -83,25 +83,32 @@ function goTo( return { currentLocation, focusSelectors }; } - let focusSelectorsCopy; + let focusSelectorsCopy: typeof focusSelectors | undefined; + function getFocusSelectorsCopy() { + focusSelectorsCopy = + focusSelectorsCopy ?? new Map( state.focusSelectors ); + return focusSelectorsCopy; + } // Set a focus selector that will be used when navigating // back to the current location. if ( focusTargetSelector && currentLocation?.path ) { - if ( ! focusSelectorsCopy ) { - focusSelectorsCopy = new Map( state.focusSelectors ); - } - focusSelectorsCopy.set( currentLocation.path, focusTargetSelector ); + getFocusSelectorsCopy().set( + currentLocation.path, + focusTargetSelector + ); } // Get the focus selector for the new location. let currentFocusSelector; - if ( isBack ) { - if ( ! focusSelectorsCopy ) { - focusSelectorsCopy = new Map( state.focusSelectors ); + if ( focusSelectors.get( path ) ) { + if ( isBack ) { + // Use the found focus selector only when navigating back. + currentFocusSelector = focusSelectors.get( path ); } - currentFocusSelector = focusSelectorsCopy.get( path ); - focusSelectorsCopy.delete( path ); + // Make a copy of the focusSelectors map to remove the focus selector + // only if necessary (ie. a focus selector was found). + getFocusSelectorsCopy().delete( path ); } return { From c8a3f44345e0ae5c1cdcde05c80f054a57625eba Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Mon, 26 Aug 2024 13:57:59 +0200 Subject: [PATCH 5/5] =?UTF-8?q?Remove=20unnecessary=20optional=20chaining?= =?UTF-8?q?=20=E2=80=94=20currentLocation=20is=20always=20defined?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../src/navigator/navigator-provider/component.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 101ab57ff6ea4d..ebcb247c574830 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -79,7 +79,7 @@ function goTo( ...restOptions } = options; - if ( currentLocation?.path === path ) { + if ( currentLocation.path === path ) { return { currentLocation, focusSelectors }; } @@ -92,7 +92,7 @@ function goTo( // Set a focus selector that will be used when navigating // back to the current location. - if ( focusTargetSelector && currentLocation?.path ) { + if ( focusTargetSelector && currentLocation.path ) { getFocusSelectorsCopy().set( currentLocation.path, focusTargetSelector @@ -130,7 +130,7 @@ function goToParent( ) { const { screens, focusSelectors } = state; const currentLocation = { ...state.currentLocation, isInitial: false }; - const currentPath = currentLocation?.path; + const currentPath = currentLocation.path; if ( currentPath === undefined ) { return { currentLocation, focusSelectors }; } @@ -186,7 +186,7 @@ function routerReducer( } // Compute the matchedPath - const currentPath = currentLocation?.path; + const currentPath = currentLocation.path; matchedPath = currentPath !== undefined ? patternMatch( currentPath, screens )