Site Editor: Remove the details pages #61741
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
Mamaduka,
oandregal,
jsnajdr,
jameskoster,
jasmussen,
ntsekouras,
fabiankaegy,
t-hamano and
a team
|
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. |
| import SidebarNavigationScreenTemplatesBrowse from '../sidebar-navigation-screen-templates-browse'; | ||
| import SidebarNavigationScreenTemplate from '../sidebar-navigation-screen-template'; | ||
| import SidebarNavigationScreenPattern from '../sidebar-navigation-screen-pattern'; |
There was a problem hiding this comment.
These components have been removed entirely, I still need to check if they have dependencies that are only used in the components themselves to avoid leftovers.
| export default function DataViewsSidebarContent() { | ||
| const { | ||
| params: { path, activeView = 'all', isCustom = 'false' }, | ||
| params: { postType, activeView = 'all', isCustom = 'false' }, |
There was a problem hiding this comment.
I think we should unify between "activeView" in the pages page and "categoryId" in the patterns page.
There was a problem hiding this comment.
Maybe we should do that when we actually use views in patterns page to avoid creating extra confusion with nomenclature.
There was a problem hiding this comment.
Isn't it just the "default views" of patterns though, just like we have "default views" for pages (regardless of whether you can create views)
There was a problem hiding this comment.
Semantically yes, technically no. With nomenclature confusion I meant mostly towards devs.
| @@ -29,23 +29,23 @@ export default function SidebarNavigationItem( { | |||
| icon, | |||
| withChevron = false, | |||
| suffix, | |||
| path, | |||
There was a problem hiding this comment.
Previously the path was a string and being used as a unique id for the "focus handling". I added an explicit uid to address the fact that now "params" is an object (I didn't want to use the postType directly as I wanted to support any link) cc @jsnajdr
There was a problem hiding this comment.
Is this uid data? Would a data- attribute make more sense? I was wondering if it's an id why not use the id attribute then?
| @@ -176,9 +170,6 @@ function useGlobalStylesOpenCssCommands() { | |||
| const { openGeneralSidebar, setEditorCanvasContainerView, setCanvasMode } = | |||
| unlock( useDispatch( editSiteStore ) ); | |||
| const { params } = useLocation(); | |||
| const isMobileViewport = useViewportMatch( 'medium', '<' ); | |||
| const isListPage = getIsListPage( params, isMobileViewport ); | |||
There was a problem hiding this comment.
This function was already broken before this PR. I changed the logic a bit to navigate to the "global styles" pages only when no active post/template is selected (no postId in the url)
|
Size Change: -5.33 kB (-0.31%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
packages/edit-site/src/components/sidebar-navigation-screen-template/home-template-details.js
Show resolved
Hide resolved
|
It's the right time to do it, so I'm happy to move forward. It certainly makes the navigation much clearer:
|
|
I did a first round of testing and seems to work quite well 🚀 . There are things to iron out, but lots of testing will be needed here. |
|
@jasmussen @jameskoster Here's an interesting challenge that we might not have thought about:
This we can't implement at the moment and to be honest, I'm not sure what the ideal scenario for all cases is, knowing that:
I do wonder whether we should actually just simplify the "site icon" to mean just go back to the listing of the pages (kind of like classic wp-admin or post editor) |
|
Yeah there are many similar flows; Adding filters, changing layout, toggling fields, sorting, switching to a different view (Drafts). In all of these cases you'd probably expect the W button to return you to exactly where you came from. The W button is a complex beast, and I don't suppose this is the PR to figure out all it's behaviors. The suggestion (always returns to list) seems okay for now. |
|
Should we move forward here, what's left? |
| locationCache.clear(); | ||
| locationCache.set( location.search, ret ); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
I'm slightly confused by this 🙂 If we clear() the locationCache all the time like here, it will ever remember only the last value, and the map will always have just one record, right? That's a common way of memoization, but usually not implemented with a Map.
Also, it works only as long as all our routing is happening inside the query params. When you start routing also with a real path, and the location changes from:
{ path: '/site-editor', search: 'postId=1' }
to
{ path '/edit', search: 'postId=1' }
then getLocationWithParams will return the old /site-editor location, because that's what's been memoized under the postId=1 key.
There was a problem hiding this comment.
you're right, for the path, we actually don't care about it but initially when I tried using weak map and use the entire location object as a key, the component was re-rendering way too much (location didn't seem stable for some reason, see this commit 62b7061
Happy to use an alternative implementation, anything you have in mind?
There was a problem hiding this comment.
Happy to use an alternative implementation, anything you have in mind?
For now you can leave it as it is, I'm working on a parallel PR that moves getLocationWithParams to the history object, there are a few other things that will be fixed by this.
initially when I tried using weak map and use the entire location object as a key, the component was re-rendering way too much
This is strange because the history package returns stable and frozen location objects. I'll try out the commit with weakmap.
There was a problem hiding this comment.
Alternative implementation proposed here: #61823.
There was a problem hiding this comment.
Left a comment there, it seems your implementation suffers from the same issue I had here with weakmap. Going to merge this PR but happy to iterate on this in your PR.
There was a problem hiding this comment.
Somehow this PR undoes #61275 and makes navigating to a navigation also change the content in the frame.
|
@draganescu Can you explain more, in my testing it's behaving exactly like shown in the video of the linked PR, so I'm not entirely sure what to expect |
In this PR
CleanShot.2024-05-21.at.13.46.36.mp4In trunk
CleanShot.2024-05-21.at.13.50.13.mp4 |
|
Flaky tests detected in 7fec893. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9173140463
|
|
@draganescu The behavior in trunk is not consistent. When you have just a single menu, you can see that going back from editor to navigation you pass by the state of the "current navigation menu" selected in the frame too. the behavior in the PR is consistent though. So I think I prefer the behavior on this PR personally. |
|
No, the inconsistency is a bug that should be fixed, but the fix is not to undo the PR. That PR had plenty of conversation around why the single post type editing for navigation menus should not be a default experience. |
|
Personally I also prefer the routing of menus here and think we should avoid the inconsistency.. |
There was a problem hiding this comment.
This is working well enough for me.
Andrei notes that this undoes work in #61275. That work came from a place to improve navigation, which is valid: it's clear that the navigation section is not in a great place. Nevertheless there's an issue where you can still edit that navigation in isolation, then press "Back", and land in a pretty incongruent state:
It's not clear that aspect was accounted for in 61275, and to me it doesn't seem an acceptable compromise. Not sure the best path forward, happy to defer to others. But at the moment, the behavior for navigation in this branch, and consequent in 6.5 which it is similar to, seems better than what's shipping in trunk.
I'm pretty sure the "Edit" dropdown to engage edit mode was intentionally left in, as a method for getting to the edit view of the navigation menu. Both are not entirely ideal, though we don't have a plan together for making the navigation edit view decent. The intention behind #61275 was to reduce the likelihood of ending up in the navigation edit view, where it's wholly unfinished and confusing—as it very well may not represent the way that particular navigation block looks/is placed. That's why the inconsistency was introduced; to prevent previewing the navigation in a confusing experience. |
|
Looks like this PR caused a performance regression for site editor first block load. Any idea what it could be? |
github-actions
bot
added
the
[Status] In Progress
kevin940726
removed
the
[Status] In Progress
|
I think this has changed the "Edit site" links behavior in the WP admin bar. It should open the currently rendered template in "edit" mode; now, it opens the template list view. ScreencastCleanShot.2024-05-22.at.11.21.07.mp4 |
|
@ellatrix I change the url to check for the "first block" metric to canvas "edit" in this PR. The reason is that this url exists similarly in both this version and the old version of the site editor. I'm guessing that the "editor frame" is probably slower to load recently which might have caused the jump in performance. So I don't really think there's a performance regression in this PR per se but I'm not sure what to do here. If I restore the previous url, we'll be testing two different things: the details page that was removed with the page list (with the page visible in the frame). @Mamaduka Good catch, I'll see if I can fix this. |
Related to #59689
What?
This PR is kind of the latest step of the unification of the "details panels" and the "inspector panel". Now that all/most of these panels unification is done, we can actually remove the "details pages" entirely.
That said, it's not without challenges when it comes to "routing" and "urls": Pages in the site editor can be loaded using two URLs today:
As we're removing the details panel step, we need to unify. We have to pick one URL and redirect the other. I decided to keep the former as it's more friendly.
This also means a change in all places where we link to these pages...
That said, it brings a lot of consistency and simplicity to our pages IMO, so I think it's a good change anyway.
Testing Instructions
This is a very impactful change in the site editor, we need to test all the flows properly. E2E tests should help us catch some but probably not all the potential regressions.