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.

Sign up for GitHub

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

Jump to bottom

Navigator: fix isInitial, refine focusSelector logic #64786

Merged
merged 5 commits into from
Aug 26, 2024
Merged

Navigator: fix isInitial, refine focusSelector logic #64786

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
09b48d6
Fix isInitial logic
ciampo Aug 25, 2024
bfaccad
Use '@wordpress/warning' instead of console.warn
ciampo Aug 25, 2024
3e6dcbd
Improved destructuring and isInitial assignment
ciampo Aug 25, 2024
fe16bbf
Refactor focus selector logic, better cleanup, lazier map copy
ciampo Aug 25, 2024
c8a3f44
Remove unnecessary optional chaining — currentLocation is always defined
ciampo Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 37 additions & 29 deletions packages/components/src/navigator/navigator-provider/component.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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.`
);
Expand All @@ -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
Expand All @@ -78,29 +79,36 @@ function goTo(
...restOptions
} = options;

if ( currentLocation?.path === path ) {
if ( currentLocation.path === path ) {
return { currentLocation, focusSelectors };
Copy link
Member

Choose a reason for hiding this comment

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

If I'm at initial location /foo and call goTo( '/foo' ), it will reset the isInitial flag and that's all. Isn't it going to fire an animation? Like the same UI sliding from right "into itself"?

By the way, all the ?.path optional chainings are not needed, .path is enough.

Copy link
Contributor Author

@ciampo ciampo Aug 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If I'm at initial location /foo and call goTo( '/foo' ), it will reset the isInitial flag and that's all. Isn't it going to fire an animation? Like the same UI sliding from right "into itself"?

I don't think "to same screen" navigations have ever been supported — for the animation to work, we'd need to create a clone of the current screen to transition to.

This is true also for "parametrised" screens. For example, if we have a <Navigator.Screen path="/product/:id" />, navigating from /product/1 to product/2 won't currently trigger an animation.

I don't think this has ever been a requirement, and that's why it's not supported.

By the way, all the ?.path optional chainings are not needed, .path is enough.

Good point, I pushed a commit removing those optional chainings

}

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 );
if ( focusTargetSelector && currentLocation.path ) {
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 {
Expand All @@ -120,8 +128,9 @@ function goToParent(
state: RouterState,
options: NavigateToParentOptions = {}
) {
const { currentLocation, screens, focusSelectors } = state;
const currentPath = currentLocation?.path;
const { screens, focusSelectors } = state;
const currentLocation = { ...state.currentLocation, isInitial: false };
const currentPath = currentLocation.path;
if ( currentPath === undefined ) {
return { currentLocation, focusSelectors };
}
Expand Down Expand Up @@ -154,21 +163,20 @@ function routerReducer(
screens = removeScreen( state, action.screen );
break;
case 'goto':
const goToNewState = goTo( state, action.path, action.options );
currentLocation = goToNewState.currentLocation;
focusSelectors = goToNewState.focusSelectors;
( { currentLocation, focusSelectors } = goTo(
state,
action.path,
action.options
) );
break;
case 'gotoparent':
const goToParentNewState = goToParent( state, action.options );
currentLocation = goToParentNewState.currentLocation;
focusSelectors = goToParentNewState.focusSelectors;
( { currentLocation, focusSelectors } = goToParent(
state,
action.options
) );
break;
}

if ( currentLocation?.path === state.initialPath ) {
currentLocation = { ...currentLocation, isInitial: true };
}

// Return early in case there is no change
if (
screens === state.screens &&
Expand All @@ -178,7 +186,7 @@ function routerReducer(
}

// Compute the matchedPath
const currentPath = currentLocation?.path;
const currentPath = currentLocation.path;
matchedPath =
currentPath !== undefined
? patternMatch( currentPath, screens )
Expand Down Expand Up @@ -220,7 +228,7 @@ function UnconnectedNavigatorProvider(
initialPathProp,
( path ) => ( {
screens: [],
currentLocation: { path },
currentLocation: { path, isInitial: true },
matchedPath: undefined,
focusSelectors: new Map(),
initialPath: initialPathProp,
Expand Down
Loading