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

Site Editor: Make sidebar back button go *back* instead of *up* if possible #52456

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Site Editor: Make sidebar back button go *back* instead of *up* if possible #52456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e5d49e
Navigator: Add replace option to goTo() and goToParent()
noisysocks Jul 13, 2023
e83e1af
Site Editor: Make sidebar back button go back instead of up if possible
noisysocks Jul 13, 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
1 change: 1 addition & 0 deletions packages/components/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Enhancements

- `TextControl`: Add `id` prop to allow for custom IDs in `TextControl`s ([#52028](https://github.com/WordPress/gutenberg/pull/52028)).
- `Navigator`: Add `replace` option to `navigator.goTo()` and `navigator.goToParent()` ([#52456](https://github.com/WordPress/gutenberg/pull/52456)).

### Bug Fix

Expand Down
37 changes: 22 additions & 15 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 @@ -148,6 +148,7 @@ function UnconnectedNavigatorProvider(
focusTargetSelector,
isBack = false,
skipFocus = false,
replace = false,
...restOptions
} = options;

Expand All @@ -172,34 +173,38 @@ function UnconnectedNavigatorProvider(
skipFocus,
};

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

return [
...prevLocationHistory.slice(
prevLocationHistory.length > MAX_HISTORY_LENGTH - 1
? 1
: 0,
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,
},
newLocation,
];
}
);
}

newLocationHistory.push( newLocation );

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

const goToParent: NavigatorContextType[ 'goToParent' ] =
useCallback( () => {
const goToParent: NavigatorContextType[ 'goToParent' ] = useCallback(
( options = {} ) => {
const currentPath =
currentLocationHistory.current[
currentLocationHistory.current.length - 1
Expand All @@ -214,8 +219,10 @@ function UnconnectedNavigatorProvider(
if ( parentPath === undefined ) {
return;
}
goTo( parentPath, { isBack: true } );
}, [ goTo ] );
goTo( parentPath, { ...options, isBack: true } );
},
[ goTo ]
);

const navigatorContextValue: NavigatorContextType = useMemo(
() => ( {
Expand Down
5 changes: 4 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 @@ -14,8 +14,11 @@ export type NavigateOptions = {
focusTargetSelector?: string;
isBack?: boolean;
skipFocus?: boolean;
replace?: boolean;
};

export type NavigateToParentOptions = Omit< NavigateOptions, 'isBack' >;

export type NavigatorLocation = NavigateOptions & {
isInitial?: boolean;
path?: string;
Expand All @@ -28,7 +31,7 @@ export type Navigator = {
params: MatchParams;
goTo: ( path: string, options?: NavigateOptions ) => void;
goBack: () => void;
goToParent: () => void;
goToParent: ( options?: NavigateToParentOptions ) => void;
};

export type NavigatorContext = Navigator & {
Expand Down
19 changes: 13 additions & 6 deletions packages/edit-site/src/components/sidebar-navigation-screen/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import {
__experimentalHStack as HStack,
__experimentalHeading as Heading,
__experimentalNavigatorToParentButton as NavigatorToParentButton,
__experimentalUseNavigator as useNavigator,
__experimentalVStack as VStack,
} from '@wordpress/components';
Expand Down Expand Up @@ -41,7 +40,7 @@ export default function SidebarNavigationScreen( {
};
}, [] );
const { getTheme } = useSelect( coreStore );
const { goTo } = useNavigator();
const navigator = useNavigator();
const theme = getTheme( currentlyPreviewingTheme() );
const icon = isRTL() ? chevronRight : chevronLeft;

Expand All @@ -58,16 +57,24 @@ export default function SidebarNavigationScreen( {
className="edit-site-sidebar-navigation-screen__title-icon"
>
{ ! isRoot && ! backPath && (
<NavigatorToParentButton
as={ SidebarButton }
icon={ isRTL() ? chevronRight : chevronLeft }
<SidebarButton
onClick={ () => {
if ( navigator.location.isInitial ) {
navigator.goToParent( { replace: true } );
} else {
navigator.goBack();
}
} }
icon={ icon }
label={ __( 'Back' ) }
showTooltip={ false }
/>
) }
{ ! isRoot && backPath && (
<SidebarButton
onClick={ () => goTo( backPath, { isBack: true } ) }
onClick={ () =>
navigator.goTo( backPath, { isBack: true } )
}
icon={ icon }
label={ __( 'Back' ) }
showTooltip={ false }
Expand Down
39 changes: 23 additions & 16 deletions packages/edit-site/src/components/sync-state-with-url/use-sync-path-with-url.js
View file
Open in desktop
Copy link
Member Author

Choose a reason for hiding this comment

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

Set GitHub to "hide whitespace" when reviewing this file.

Copy link
Member

Choose a reason for hiding this comment

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

The link if anyone is interested: https://github.com/WordPress/gutenberg/pull/52456/files?w=1

Copy link
Member

Choose a reason for hiding this comment

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

TIL there's a GUI for it 🤣

Screenshot 2023-07-11 at 1 26 04 pm

Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,12 @@ export function getPathFromURL( urlParams ) {
return path;
}

function isSubset( subset, superset ) {
Copy link
Member

Choose a reason for hiding this comment

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

Just curious: this function, on the surface, appears to check equality. Is it possible to add a note to explain the relationship between subset and superset?

Or just in this comment box for me 🤣

Copy link
Member Author

Choose a reason for hiding this comment

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

If all of the key/value pairs of A are also present in B then A is a subset of B and B is a superset of A.

https://en.wikipedia.org/wiki/Subset

Maybe I should call it includes which is what lodash used? Would that be more familiar?

https://lodash.com/docs/4.17.15#includes

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I should call it includes which is what lodash used? Would that be more familiar?

I was only thinking of the casual observer, especially since this file's code is, or at least has been, at best, difficult to grok at first glance.

I'm fine with the function description personally. Thanks for the explainer! 🍺

return Object.entries( subset ).every( ( [ key, value ] ) => {
return superset[ key ] === value;
} );
}

export default function useSyncPathWithURL() {
const history = useHistory();
const { params: urlParams } = useLocation();
Expand All @@ -44,11 +50,10 @@ export default function useSyncPathWithURL() {
params: navigatorParams,
goTo,
} = useNavigator();
const currentUrlParams = useRef( urlParams );
const currentPath = useRef( navigatorLocation.path );
const isMounting = useRef( true );

useEffect( () => {
useEffect(
() => {
// The navigatorParams are only initially filled properly when the
// navigator screens mount. so we ignore the first synchronisation.
if ( isMounting.current ) {
Expand All @@ -57,18 +62,13 @@ export default function useSyncPathWithURL() {
}

function updateUrlParams( newUrlParams ) {
if (
Object.entries( newUrlParams ).every( ( [ key, value ] ) => {
return currentUrlParams.current[ key ] === value;
} )
) {
if ( isSubset( newUrlParams, urlParams ) ) {
return;
}
const updatedParams = {
...currentUrlParams.current,
...urlParams,
...newUrlParams,
};
currentUrlParams.current = updatedParams;
history.push( updatedParams );
}

Expand Down Expand Up @@ -106,14 +106,21 @@ export default function useSyncPathWithURL() {
: navigatorLocation.path,
} );
}
}, [ navigatorLocation?.path, navigatorParams, history ] );
},
// Trigger only when navigator changes to prevent infinite loops.
// eslint-disable-next-line react-hooks/exhaustive-deps
[ navigatorLocation?.path, navigatorParams ]
);

useEffect( () => {
currentUrlParams.current = urlParams;
useEffect(
() => {
const path = getPathFromURL( urlParams );
if ( currentPath.current !== path ) {
currentPath.current = path;
if ( navigatorLocation.path !== path ) {
goTo( path );
}
}, [ urlParams, goTo ] );
},
// Trigger only when URL changes to prevent infinite loops.
// eslint-disable-next-line react-hooks/exhaustive-deps
[ urlParams ]
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify the changes in this hook. The hook is supposed to be independent of the navigation, just synchronizes changes, so I'm not sure how it's related to this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep so the key change, and all that this PR needs at minimum, is changing this:

gutenberg/packages/edit-site/src/components/sync-state-with-url/use-sync-path-with-url.js

Lines 113 to 117 in 181b49c

const path = getPathFromURL( urlParams );
if ( currentPath.current !== path ) {
currentPath.current = path;
goTo( path );
}

To this:

gutenberg/packages/edit-site/src/components/sync-state-with-url/use-sync-path-with-url.js

Lines 117 to 120 in e83e1af

const path = getPathFromURL( urlParams );
if ( navigatorLocation.path !== path ) {
goTo( path );
}

The former is buggy. When something calls navigator.goTo, e.g. when you click Pages, the result is:

  1. The navigator path changes to /page
  2. The first useEffect fires (because the navigator path changed) and calls history.push to change the URL to ?path=%2Fpage
  3. The second useEffect fires (because the URL params changed) and calls navigator.goTo to navigate to /page

(3) is extraneous. We don't need to call navigator.goTo( '/page' ) as the navigator location is already /page. Doing this results in two history items within the navigator and means that the user has to click back twice to return to the root page.

The simplest way to fix this would be to update currentPath.current in the first useEffect, but I thought I'd go further and attempt to simplify useSyncPathWithURL to not require the currentPath and currentUrlParams refs at all. They duplicate state that is already in the useHistory and useNavigator hooks. Having the refs makes the hook more difficult to understand and makes it easy for bugs of this nature to slip by.

}
Loading