Skip to content
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

Navigator: remove location history, simplify internal logic #64675

Merged
merged 11 commits into from
Aug 23, 2024
5 changes: 5 additions & 0 deletions packages/components/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)).
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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`

Expand All @@ -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[] >`

Expand Down
173 changes: 90 additions & 83 deletions packages/components/src/navigator/navigator-provider/component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,110 +37,97 @@ 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.`
);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use @wordpress/warning for this, already used widely in the components package.

return screens;
}
return [ ...screens, screen ];
}

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 );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I would enhance the logic for the case when we navigate to the path in a isBack = false way, and it has a focusSelector stored. Then we don't want to focus (that's implemented currently) and also to delete the stored focusSelector because it's no longer relevant.

Also, a little optimization opportunity: create the copy only when you really found a selector to be deleted. Currently, if .get( path ) is null, you created the copy for nothing, it won't be mutated.

}

newLocationHistory.push( newLocation );

return newLocationHistory;
return {
currentLocation: {
...restOptions,
path,
isBack,
hasRestoredFocus: false,
focusTargetSelector: currentFocusSelector,
skipFocus,
},
ciampo marked this conversation as resolved.
Show resolved Hide resolved
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,
Expand All @@ -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 );
Expand All @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can do a destructuring assignment in only you wrap it in parens so that it's not confused with a block:

( { currentLocation, focusSelection } = goTo() );

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 };
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want to do this? isInitial should be true only when the page is initially loaded. Then we don't want to do animations or change focus. But when later navigating to the initial page, the initial page is a page like any other, we want to run animations and to restore focus.

Copy link
Contributor Author

@ciampo ciampo Aug 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really want to do this?

No, the logic is wrong. I had already noticed it while working on #64777. I'm going to extract and refine the fix, and apply it in a separate PR


// 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 )
Expand All @@ -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,
} )
);

Expand Down Expand Up @@ -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();
Expand Down
4 changes: 3 additions & 1 deletion packages/components/src/navigator/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Expand Down
Loading