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

Try controlled navigator behavior #51915

Closed
wants to merge 3 commits into from
+204 −79
Closed

Try controlled navigator behavior #51915

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4637011
Try controlled navigator behavior
youknowriad Jun 26, 2023
8f6bf7c
updatedLocation => updateLocation
ciampo Nov 6, 2023
2ab90a0
Formatting, commenting
ciampo Nov 6, 2023
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
194 changes: 116 additions & 78 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 @@ -32,6 +32,7 @@ import type {
Screen,
} from '../types';
import { patternMatch, findParent } from '../utils/router';
import { useControlledValue } from '../../utils';

type MatchedPath = ReturnType< typeof patternMatch >;
type ScreenAction = { type: string; screen: Screen };
Expand All @@ -56,16 +57,23 @@ function UnconnectedNavigatorProvider(
props: WordPressComponentProps< NavigatorProviderProps, 'div' >,
forwardedRef: ForwardedRef< any >
) {
const { initialPath, children, className, ...otherProps } =
useContextSystem( props, 'NavigatorProvider' );
const {
initialPath,
location: locationProp,
onChange,
children,
className,
...otherProps
} = useContextSystem( props, 'NavigatorProvider' );
const [ location, updateLocation ] = useControlledValue( {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to keep both location and locationHistory internal states? I'm afraid it would cause extra re-renders and potentially some race conditions too. Maybe we can just keep the location history as a ref, and update navigatorContextValue accordingly? (I know that this would probably also require refactoring the logic around the new useEffect)

onChange,
value: locationProp,
defaultValue: { path: initialPath },
} );

const [ locationHistory, setLocationHistory ] = useState<
NavigatorLocation[]
>( [
{
path: initialPath,
},
] );
>( [ location ?? { path: initialPath } ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

Given that useControlledValue is already handling the fallback to { path: initialPath } as the default value when locationProp is not defined, I believe that this fallback is extra and can be removed

Suggested change
>( [ location ?? { path: initialPath } ] );
>( location ? [ location ] : [] );

const currentLocationHistory = useRef< NavigatorLocation[] >( [] );
const [ screens, dispatch ] = useReducer( screensReducer, [] );
const currentScreens = useRef< Screen[] >( [] );
Expand All @@ -75,6 +83,90 @@ function UnconnectedNavigatorProvider(
useEffect( () => {
currentLocationHistory.current = locationHistory;
}, [ locationHistory ] );
useEffect( () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

There seems to be a bug that causes the component to add the initial location twice to the location history when loading. This also causes the isInitial flag added in navigatorContextValue to be false even when it should be true, which causes the initial NavigatorScreen to animate on first render (when it shouldn't)

if ( ! location ) {
return;
}

const {
focusTargetSelector,
isBack = false,
skipFocus = false,
replace = false,
path: destinationPath,
...restOptions
} = location;

const isNavigatingToPreviousPath =
isBack &&
currentLocationHistory.current.length > 1 &&
currentLocationHistory.current[
currentLocationHistory.current.length - 2
].path === destinationPath;

if ( isNavigatingToPreviousPath ) {
// Navigating back to previous location
setLocationHistory( ( prevLocationHistory ) => {
if ( prevLocationHistory.length <= 1 ) {
return prevLocationHistory;
}
return [
...prevLocationHistory.slice( 0, -2 ),
{
...prevLocationHistory[
prevLocationHistory.length - 2
],
isBack: true,
hasRestoredFocus: false,
},
];
} );
} else {
// Navigating to a new location
setLocationHistory( ( prevLocationHistory ) => {
const newLocation = {
...restOptions,
path: destinationPath,
isBack,
hasRestoredFocus: false,
skipFocus,
};

if ( prevLocationHistory.length === 0 ) {
return replace ? [] : [ newLocation ];
}

// Form the new location history array.
// Start by picking all previous history items, apart from the last one.
// A check is in place to make sure that the array doesn't grow
// beyond a max length.
const newLocationHistory = prevLocationHistory.slice(
prevLocationHistory.length > MAX_HISTORY_LENGTH - 1 ? 1 : 0,
-1
);

// If we're not replacing history, add the last location history item (the
// one what was just navigated from). We also assign it a
// `focusTargetSelector` for enhanced focus restoration when navigating
// back to it.
if ( ! replace ) {
newLocationHistory.push( {
...prevLocationHistory[
prevLocationHistory.length - 1
],
focusTargetSelector,
} );
}

// In any case, append the new location to the array (the one that
// was just navigated to)
newLocationHistory.push( newLocation );

return newLocationHistory;
} );
}
}, [ location ] );

const currentMatch = useRef< MatchedPath >();
const matchedPath = useMemo( () => {
let currentPath: string | undefined;
Expand All @@ -88,8 +180,8 @@ function UnconnectedNavigatorProvider(
return undefined;
}

const resolvePath = ( path: string ) => {
const newMatch = patternMatch( path, screens );
const resolvePath = ( pathToResolve: string ) => {
const newMatch = patternMatch( pathToResolve, screens );

// If the new match is the same as the current match,
// return the previous one for performance reasons.
Expand Down Expand Up @@ -124,80 +216,26 @@ function UnconnectedNavigatorProvider(
);

const goBack: NavigatorContextType[ 'goBack' ] = useCallback( () => {
setLocationHistory( ( prevLocationHistory ) => {
if ( prevLocationHistory.length <= 1 ) {
return prevLocationHistory;
}
return [
...prevLocationHistory.slice( 0, -2 ),
{
...prevLocationHistory[ prevLocationHistory.length - 2 ],
isBack: true,
hasRestoredFocus: false,
},
];
if ( currentLocationHistory.current.length < 2 ) {
return;
}

updateLocation( {
isBack: true,
path: currentLocationHistory.current[
currentLocationHistory.current.length - 2
].path,
} );
}, [] );
}, [ updateLocation ] );

const goTo: NavigatorContextType[ 'goTo' ] = useCallback(
( path, options = {} ) => {
const {
focusTargetSelector,
isBack = false,
skipFocus = false,
replace = false,
...restOptions
} = options;

const isNavigatingToPreviousPath =
isBack &&
currentLocationHistory.current.length > 1 &&
currentLocationHistory.current[
currentLocationHistory.current.length - 2
].path === path;

if ( isNavigatingToPreviousPath ) {
goBack();
return;
}

setLocationHistory( ( prevLocationHistory ) => {
const newLocation = {
...restOptions,
path,
isBack,
hasRestoredFocus: false,
skipFocus,
};

if ( prevLocationHistory.length === 0 ) {
return replace ? [] : [ newLocation ];
}

const newLocationHistory = prevLocationHistory.slice(
prevLocationHistory.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).
{
...prevLocationHistory[
prevLocationHistory.length - 1
],
focusTargetSelector,
}
);
}

newLocationHistory.push( newLocation );

return newLocationHistory;
( destinationPath, options = {} ) => {
updateLocation( {
...options,
path: destinationPath,
} );
},
[ goBack ]
[ updateLocation ]
);

const goToParent: NavigatorContextType[ 'goToParent' ] = useCallback(
Expand Down
74 changes: 74 additions & 0 deletions packages/components/src/navigator/stories/index.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@
*/
import type { Meta, StoryFn } from '@storybook/react';

/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';

/**
* Internal dependencies
*/
Expand All @@ -18,6 +23,7 @@ import {
NavigatorToParentButton,
useNavigator,
} from '..';
import type { NavigatorLocation } from '../types';

const meta: Meta< typeof NavigatorProvider > = {
component: NavigatorProvider,
Expand Down Expand Up @@ -364,3 +370,71 @@ SkipFocus.args = {
</>
),
};

export const ControlledNavigator: StoryFn< typeof NavigatorProvider > = (
props
) => {
const [ location, setLocation ] = useState< NavigatorLocation >( {
path: props.initialPath ?? '/',
} );
return (
<NavigatorProvider
{ ...props }
location={ location }
onChange={ setLocation }
style={ { ...props.style, height: '100vh', maxHeight: '450px' } }
>
<NavigatorScreen path="/">
<Card>
<CardBody>
<NavigatorButton variant="secondary" path="/child1">
Go to first child.
</NavigatorButton>
<NavigatorButton variant="secondary" path="/child2">
Go to second child.
</NavigatorButton>
</CardBody>
</Card>
</NavigatorScreen>
<NavigatorScreen path="/child1">
<Card>
<CardBody>
This is the first child
<NavigatorToParentButton variant="secondary">
Go back to parent
</NavigatorToParentButton>
</CardBody>
</Card>
</NavigatorScreen>
<NavigatorScreen path="/child2">
<Card>
<CardBody>
This is the second child
<NavigatorToParentButton variant="secondary">
Go back to parent
</NavigatorToParentButton>
<NavigatorButton
variant="secondary"
path="/child2/grandchild"
>
Go to grand child.
</NavigatorButton>
</CardBody>
</Card>
</NavigatorScreen>
<NavigatorScreen path="/child2/grandchild">
<Card>
<CardBody>
This is the grand child
<NavigatorToParentButton variant="secondary">
Go back to parent
</NavigatorToParentButton>
</CardBody>
</Card>
</NavigatorScreen>
</NavigatorProvider>
);
};
ControlledNavigator.args = {
initialPath: '/child2/grandchild',
};
15 changes: 14 additions & 1 deletion packages/components/src/navigator/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,24 @@ export type NavigatorProviderProps = {
/**
* The initial active path.
*/
initialPath: string;
initialPath?: string;

/**
* The children elements.
*/
children: ReactNode;

/**
* The current path. When provided the navigator will be controlled.
*/
location?: NavigatorLocation;
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably change path in the NavigatorLocation type to be mandatory


/**
* Navigates to a new path.
*
* @param path The path to navigate to.
*/
onChange?: ( location: NavigatorLocation ) => void;
};

export type NavigatorScreenProps = {
Expand Down