-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Site Editor sidebar: provide explicit backPaths, remove the getBackPath helper #61286
Changes from all commits
75b233b
632ba26
2602dbd
40f4a03
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -22,7 +22,7 @@ const { useLocation } = unlock( routerPrivateApis ); | |||||
|
||||||
export const postType = `wp_navigation`; | ||||||
|
||||||
export default function SidebarNavigationScreenNavigationMenu() { | ||||||
export default function SidebarNavigationScreenNavigationMenu( { backPath } ) { | ||||||
const { | ||||||
params: { postId }, | ||||||
} = useLocation(); | ||||||
|
@@ -67,6 +67,7 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||||
description={ __( | ||||||
'Navigation Menus are a curated collection of blocks that allow visitors to get around your site.' | ||||||
) } | ||||||
backPath={ backPath } | ||||||
> | ||||||
<Spinner className="edit-site-sidebar-navigation-screen-navigation-menus__loading" /> | ||||||
</SidebarNavigationScreenWrapper> | ||||||
|
@@ -77,6 +78,7 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||||
return ( | ||||||
<SidebarNavigationScreenWrapper | ||||||
description={ __( 'Navigation Menu missing.' ) } | ||||||
backPath={ backPath } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
/> | ||||||
); | ||||||
} | ||||||
|
@@ -93,6 +95,7 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||||
onDuplicate={ _handleDuplicate } | ||||||
/> | ||||||
} | ||||||
backPath={ backPath } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
title={ buildNavigationLabel( | ||||||
navigationMenu?.title, | ||||||
navigationMenu?.id, | ||||||
|
@@ -106,6 +109,7 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||||
return ( | ||||||
<SingleNavigationMenu | ||||||
navigationMenu={ navigationMenu } | ||||||
backPath={ backPath } | ||||||
handleDelete={ _handleDelete } | ||||||
handleSave={ _handleSave } | ||||||
handleDuplicate={ _handleDuplicate } | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,32 +32,6 @@ import { SidebarNavigationContext } from '../sidebar'; | |
|
||
const { useHistory, useLocation } = unlock( routerPrivateApis ); | ||
|
||
function getBackPath( params ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is nice, thank you. What do you think of collocating the backPath info in the router and always pass it down as a prop to the sidebar? if ( path === '/page' ) {
return {
sidebar: <SidebarNavigationScreen backPath={ { path: '/page', postId } } />,
// ....
};
} It's not big of a difference, really, given that it'd now behave like a prop to the sidebar component. This slight change just means that we still have a centralized place (the router) with all the routing information, so it's easier maintenance. When we open this for extension or introduce an API for route registration, it may be good to provide all routing logic in that API and not require clients to provide the backpath separately, as part of their Sidebar UI component. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a great idea, I implemented it. Now all the wiring between paths and rendered components is concentrated in the router, including the backpaths. |
||
// Navigation Menus are not currently part of a data view. | ||
// Therefore when navigating back from a navigation menu | ||
// the target path is the navigation listing view. | ||
if ( params.path === '/navigation' && params.postId ) { | ||
return { path: '/navigation' }; | ||
} | ||
|
||
// From a data view path we navigate back to root | ||
if ( params.path ) { | ||
return {}; | ||
} | ||
|
||
// From edit screen for a post we navigate back to post-type specific data view | ||
if ( params.postType === 'page' ) { | ||
return { path: '/page', postId: params.postId }; | ||
} else if ( params.postType === 'wp_template' ) { | ||
return { path: '/wp_template', postId: params.postId }; | ||
} else if ( params.postType === 'wp_navigation' ) { | ||
return { path: '/navigation', postId: params.postId }; | ||
} | ||
|
||
// Go back to root by default | ||
return {}; | ||
} | ||
|
||
export default function SidebarNavigationScreen( { | ||
isRoot, | ||
title, | ||
|
@@ -89,7 +63,9 @@ export default function SidebarNavigationScreen( { | |
const location = useLocation(); | ||
const history = useHistory(); | ||
const navigate = useContext( SidebarNavigationContext ); | ||
const backPath = backPathProp ?? location.state?.backPath; | ||
const icon = isRTL() ? chevronRight : chevronLeft; | ||
|
||
return ( | ||
<> | ||
<VStack | ||
|
@@ -107,10 +83,6 @@ export default function SidebarNavigationScreen( { | |
{ ! isRoot && ( | ||
<SidebarButton | ||
onClick={ () => { | ||
const backPath = | ||
backPathProp ?? | ||
location.state?.backPath ?? | ||
getBackPath( location.params ); | ||
history.push( backPath ); | ||
navigate( 'back' ); | ||
} } | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.