From cc55c51a1307f1b06f8d0105f1efa1b1a4bef5da Mon Sep 17 00:00:00 2001 From: Marco Ciampini Date: Fri, 23 Aug 2024 19:35:08 +0200 Subject: [PATCH] Navigator: remove location history, simplify internal logic (#64675) * Remove location history and only use current location * Deprecate `options.replace` * Make sure that two screens with the same path cannot be added * Re-implement focus restoration relying on a map with paths as keys * Re-implement the isInitial flag by relying on the initialPath * Update README * Comment map deletion out * CHANGELOG * Fix broken focus restoration by updating focusSelectors in the navigator reducer * create a copy of focusSelectors map only when necessary * Set isInitial inside the reducer, instead of on the context value --- Co-authored-by: ciampo Co-authored-by: tyxla --- packages/components/CHANGELOG.md | 5 + .../navigator/navigator-provider/README.md | 5 +- .../navigator-provider/component.tsx | 173 +++++++++--------- packages/components/src/navigator/types.ts | 4 +- 4 files changed, 100 insertions(+), 87 deletions(-) diff --git a/packages/components/CHANGELOG.md b/packages/components/CHANGELOG.md index 542d2b8334ed6..44536df98b581 100644 --- a/packages/components/CHANGELOG.md +++ b/packages/components/CHANGELOG.md @@ -2,6 +2,10 @@ ## Unreleased +### Deprecations + +- Deprecate `replace` from the options accepted by `useNavigator().goTo()` ([#64675](https://github.com/WordPress/gutenberg/pull/64675)). + ### Enhancements - `ColorPicker`: Adopt radius scale ([#64693](https://github.com/WordPress/gutenberg/pull/64693)). @@ -16,6 +20,7 @@ - `TabPanel`: Remove radius applied to panel focus style ([#64693](https://github.com/WordPress/gutenberg/pull/64693)). - `Tabs`: Remove radius applied to panel focus style ([#64693](https://github.com/WordPress/gutenberg/pull/64693)). - `UnitControl`: Update unit select styles ([#64712](https://github.com/WordPress/gutenberg/pull/64712)). +- `Navigator`: remove location history, simplify internal logic ([#64675](https://github.com/WordPress/gutenberg/pull/64675)). - Decrease horizontal padding from 16px to 12px on the following components, when in the 40px default size ([#64708](https://github.com/WordPress/gutenberg/pull/64708)). - `AnglePickerControl` - `ColorPicker` (on the inputs) diff --git a/packages/components/src/navigator/navigator-provider/README.md b/packages/components/src/navigator/navigator-provider/README.md index 13745fae68a15..35bf7a69720be 100644 --- a/packages/components/src/navigator/navigator-provider/README.md +++ b/packages/components/src/navigator/navigator-provider/README.md @@ -72,7 +72,6 @@ The available options are: - `focusTargetSelector`: `string`. An optional property used to specify the CSS selector used to restore focus on the matching element when navigating back; - `isBack`: `boolean`. An optional property used to specify whether the navigation should be considered as backwards (thus enabling focus restoration when possible, and causing the animation to be backwards too); - `skipFocus`: `boolean`. An optional property used to opt out of `Navigator`'s focus management, useful when the consumer of the component wants to manage focus themselves; -- `replace`: `boolean`. An optional property used to cause the new location to replace the current location in the stack. ### `goBack`: `( path: string, options: NavigateOptions ) => void` @@ -87,8 +86,8 @@ The available options are the same as for the `goTo` method, except for the `isB The `location` object represent the current location, and has a few properties: - `path`: `string`. The path associated to the location. -- `isBack`: `boolean`. A flag that is `true` when the current location was reached by navigating backwards in the location history. -- `isInitial`: `boolean`. A flag that is `true` only for the first (root) location in the location history. +- `isBack`: `boolean`. A flag that is `true` when the current location was reached by navigating backwards. +- `isInitial`: `boolean`. A flag that is `true` only for the initial location. ### `params`: `Record< string, string | string[] >` diff --git a/packages/components/src/navigator/navigator-provider/component.tsx b/packages/components/src/navigator/navigator-provider/component.tsx index 78bc674c06fbd..8e596778c7f21 100644 --- a/packages/components/src/navigator/navigator-provider/component.tsx +++ b/packages/components/src/navigator/navigator-provider/component.tsx @@ -37,14 +37,22 @@ type RouterAction = | { type: 'gotoparent'; options?: NavigateToParentOptions }; type RouterState = { + initialPath: string; screens: Screen[]; - locationHistory: NavigatorLocation[]; + currentLocation: NavigatorLocation; matchedPath: MatchedPath; + focusSelectors: Map< string, string >; }; -const MAX_HISTORY_LENGTH = 50; - function addScreen( { screens }: RouterState, screen: Screen ) { + if ( screens.some( ( s ) => s.path === screen.path ) ) { + // eslint-disable-next-line no-console + console.warn( + `Navigator: a screen with path ${ screen.path } already exists. +The screen with id ${ screen.id } will not be added.` + ); + return screens; + } return [ ...screens, screen ]; } @@ -52,95 +60,74 @@ function removeScreen( { screens }: RouterState, screen: Screen ) { return screens.filter( ( s ) => s.id !== screen.id ); } -function goBack( { locationHistory }: RouterState ) { - if ( locationHistory.length <= 1 ) { - return locationHistory; - } - return [ - ...locationHistory.slice( 0, -2 ), - { - ...locationHistory[ locationHistory.length - 2 ], - isBack: true, - hasRestoredFocus: false, - }, - ]; -} - function goTo( state: RouterState, path: string, options: NavigateOptions = {} ) { - const { locationHistory } = state; + const { currentLocation, focusSelectors } = state; + const { - focusTargetSelector, + // Default assignments isBack = false, skipFocus = false, - replace = false, + // Extract to avoid forwarding + replace, + focusTargetSelector, + // Rest ...restOptions } = options; - const isNavigatingToSamePath = - locationHistory.length > 0 && - locationHistory[ locationHistory.length - 1 ].path === path; - if ( isNavigatingToSamePath ) { - return locationHistory; - } - - const isNavigatingToPreviousPath = - isBack && - locationHistory.length > 1 && - locationHistory[ locationHistory.length - 2 ].path === path; - - if ( isNavigatingToPreviousPath ) { - return goBack( state ); + if ( currentLocation?.path === path ) { + return { currentLocation, focusSelectors }; } - const newLocation = { - ...restOptions, - path, - isBack, - hasRestoredFocus: false, - skipFocus, - }; + let focusSelectorsCopy; - if ( locationHistory.length === 0 ) { - return replace ? [] : [ newLocation ]; + // 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 ); } - const newLocationHistory = locationHistory.slice( - locationHistory.length > MAX_HISTORY_LENGTH - 1 ? 1 : 0, - -1 - ); - - if ( ! replace ) { - newLocationHistory.push( - // Assign `focusTargetSelector` to the previous location in history - // (the one we just navigated from). - { - ...locationHistory[ locationHistory.length - 1 ], - focusTargetSelector, - } - ); + // Get the focus selector for the new location. + let currentFocusSelector; + if ( isBack ) { + if ( ! focusSelectorsCopy ) { + focusSelectorsCopy = new Map( state.focusSelectors ); + } + currentFocusSelector = focusSelectorsCopy.get( path ); + focusSelectorsCopy.delete( path ); } - newLocationHistory.push( newLocation ); - - return newLocationHistory; + return { + currentLocation: { + ...restOptions, + path, + isBack, + hasRestoredFocus: false, + focusTargetSelector: currentFocusSelector, + skipFocus, + }, + focusSelectors: focusSelectorsCopy ?? focusSelectors, + }; } function goToParent( state: RouterState, options: NavigateToParentOptions = {} ) { - const { locationHistory, screens } = state; - const currentPath = locationHistory[ locationHistory.length - 1 ].path; + const { currentLocation, screens, focusSelectors } = state; + const currentPath = currentLocation?.path; if ( currentPath === undefined ) { - return locationHistory; + return { currentLocation, focusSelectors }; } const parentPath = findParent( currentPath, screens ); if ( parentPath === undefined ) { - return locationHistory; + return { currentLocation, focusSelectors }; } return goTo( state, parentPath, { ...options, @@ -152,7 +139,13 @@ function routerReducer( state: RouterState, action: RouterAction ): RouterState { - let { screens, locationHistory, matchedPath } = state; + let { + screens, + currentLocation, + matchedPath, + focusSelectors, + ...restState + } = state; switch ( action.type ) { case 'add': screens = addScreen( state, action.screen ); @@ -161,26 +154,31 @@ function routerReducer( screens = removeScreen( state, action.screen ); break; case 'goto': - locationHistory = goTo( state, action.path, action.options ); + const goToNewState = goTo( state, action.path, action.options ); + currentLocation = goToNewState.currentLocation; + focusSelectors = goToNewState.focusSelectors; break; case 'gotoparent': - locationHistory = goToParent( state, action.options ); + const goToParentNewState = goToParent( state, action.options ); + currentLocation = goToParentNewState.currentLocation; + 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 && - locationHistory === state.locationHistory + currentLocation === state.currentLocation ) { return state; } // Compute the matchedPath - const currentPath = - locationHistory.length > 0 - ? locationHistory[ locationHistory.length - 1 ].path - : undefined; + const currentPath = currentLocation?.path; matchedPath = currentPath !== undefined ? patternMatch( currentPath, screens ) @@ -197,23 +195,35 @@ function routerReducer( matchedPath = state.matchedPath; } - return { screens, locationHistory, matchedPath }; + return { + ...restState, + screens, + currentLocation, + matchedPath, + focusSelectors, + }; } function UnconnectedNavigatorProvider( props: WordPressComponentProps< NavigatorProviderProps, 'div' >, forwardedRef: ForwardedRef< any > ) { - const { initialPath, children, className, ...otherProps } = - useContextSystem( props, 'NavigatorProvider' ); + const { + initialPath: initialPathProp, + children, + className, + ...otherProps + } = useContextSystem( props, 'NavigatorProvider' ); const [ routerState, dispatch ] = useReducer( routerReducer, - initialPath, + initialPathProp, ( path ) => ( { screens: [], - locationHistory: [ { path } ], + currentLocation: { path }, matchedPath: undefined, + focusSelectors: new Map(), + initialPath: initialPathProp, } ) ); @@ -242,19 +252,16 @@ function UnconnectedNavigatorProvider( [] ); - const { locationHistory, matchedPath } = routerState; + const { currentLocation, matchedPath } = routerState; const navigatorContextValue: NavigatorContextType = useMemo( () => ( { - location: { - ...locationHistory[ locationHistory.length - 1 ], - isInitial: locationHistory.length === 1, - }, + location: currentLocation, params: matchedPath?.params ?? {}, match: matchedPath?.id, ...methods, } ), - [ locationHistory, matchedPath, methods ] + [ currentLocation, matchedPath, methods ] ); const cx = useCx(); diff --git a/packages/components/src/navigator/types.ts b/packages/components/src/navigator/types.ts index c45762d558af2..855787b4d0a19 100644 --- a/packages/components/src/navigator/types.ts +++ b/packages/components/src/navigator/types.ts @@ -28,7 +28,9 @@ export type NavigateOptions = { */ skipFocus?: boolean; /** - * Whether the navigation should replace the current location in the stack. + * Note: this option is deprecated and currently doesn't have any effect. + * @deprecated + * @ignore */ replace?: boolean; };