-
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
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: -22 B (0%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
key: 'styles', | ||
key: 'navigation', |
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.
👍 Good spot!
@@ -61,12 +61,15 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||
const _handleSave = ( edits ) => handleSave( navigationMenu, edits ); | |||
const _handleDuplicate = () => handleDuplicate( navigationMenu ); | |||
|
|||
const backPath = { path: '/navigation', postId }; |
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.
I think this is an error (although it may already exist on trunk
).
When going back from a single Navigation Menu you should return to the listing route which is /navigation
. The postId should not be required for this.
This will alter slightly once #61275 is merged however...
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.
This is correct and intended. This backPath
does return you to to listing route which is /navigation
. The /navigation
route is in the path
field. The additional postId
specifies that we are returning from the editor for postId
and that postId
should be selected in the listing view.
In the navigations listing this is unused, but look how a DataView-enabled /page
listing looks like with the postId
parameter:
Here the postId=58
page named "Shop" is selected, and we are on a listing page, not on an editor page for "Shop".
For consistency, the postId
is included in all "listing route" backpaths no matter whether they are actually used.
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.
Yes but that assumes that Navigation uses dataviews which as yet they don't because we have no good UX for that.
Feel free to merge this "as is". Someone will just end up resolving this in the PR I referenced.
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.
What's the flow for me to load the screen where this sidebar loads?
I also don't think we should have in the URL parameters we don't need, but I want to double check I'm not missing something.
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.
OK, I removed the postId
parameter from the /navigation
link, and also from the /wp_template
link. They were harmless, but still redundant. We can add them back at the moment when the listing UI actually has use for them.
if ( isLoading ) { | ||
return ( | ||
<SidebarNavigationScreenWrapper | ||
description={ __( | ||
'Navigation Menus are a curated collection of blocks that allow visitors to get around your site.' | ||
) } | ||
backPath={ backPath } |
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.
backPath={ backPath } | |
backPath={ "/navigation" } |
@@ -77,6 +80,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 comment
The reason will be displayed to describe this comment to others. Learn more.
backPath={ backPath } | |
backPath={ "/navigation" } |
@@ -92,6 +96,7 @@ export default function SidebarNavigationScreenNavigationMenu() { | |||
onDuplicate={ _handleDuplicate } | |||
/> | |||
} | |||
backPath={ backPath } |
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.
backPath={ backPath } | |
backPath={ "/navigation" } |
@@ -32,25 +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 comment
The 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 comment
The 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.
188d60e
to
2602dbd
Compare
This PR removes the
getBackPath
helper from the Site Editor sidebar, and instead passes an explicitbackPath
to every instance ofSidebarNavigationScreen
.This moves the decision about a
backPath
from a centralized location into individual screens that should in the best position to know what it means to go "back" from them.Followup to #60466.
How to test: Check out all the Site Editor screens and verify that the back button takes you to the right place.