Site Editor: Initial routing refactoring to separate preview from list view #57938
Conversation
youknowriad
added
[Type] Enhancement
youknowriad
requested review from
oandregal,
SaxonF,
ellatrix,
kevin940726,
jorgefilipecosta and
ntsekouras
| }, | ||
| } | ||
| : {} | ||
| { areas.content && canvasMode !== 'edit' && ( |
There was a problem hiding this comment.
This is the new "content" area.
There was a problem hiding this comment.
For someone with limited exposure to site editor code, like myself, the fact that the logic to decide what to render was split in so many places made it hard to navigate and understand. I welcome an approach that centralizes the routing logic and maps it to render areas.
With that in mind, why the canvasMode shouldn't be absorbed by the useLayout hook?
There was a problem hiding this comment.
With that in mind, why the canvasMode shouldn't be absorbed by the useLayout hook?
Can you clarify a bit more what you have in mind?
There was a problem hiding this comment.
It'd be great to see something along these lines:
// The useLayoutAreas hook centralizes all logic about which area is avaliable.
function useLayoutAreas() {
if ( path === '/page' && canvasMode === 'edit' ) {
return { areas: { /* ... */ } };
}
if ( path === '/page' ) {
return { areas: { /* ... */ } };
}
// etc.
}
// The Layout component composes the layout.
export default function Layout() {
const { areas } = useLayoutAreas();
return (
<div className="edit-site-layout">
{ areas.sidebar && <div className="edit-site-layout__sidebar">{ areas.sidebar }</div> }
{ areas.content && <div className="edit-site-layout__content">{ areas.sidebar }</div> }
{ areas.preview && <div className="edit-site-layout__preview">{ areas.sidebar }</div> }
</div>
);
}There was a problem hiding this comment.
I agree that's the goal.
- We can't do sidebar now for different reasons: the navigation component need to be controlled (which is not at the moment).
- We can't ditch the canvas mode entirely (maybe later?) because it's a state that is used in very different places.
| @@ -296,82 +281,82 @@ export default function Layout() { | |||
|
|
|||
| <SavePanel /> | |||
|
|
|||
| { showCanvas && ( | |||
| <> | |||
| { isListPage && <PageMain /> } | |||
There was a problem hiding this comment.
The list page is now rendered within the "content" area while the editor is rendered within the "preview" area.
|
|
||
| const { useLocation } = unlock( routerPrivateApis ); | ||
|
|
||
| export default function useLayoutAreas() { |
There was a problem hiding this comment.
This is what you could describe as "route definitions" that would be easy to swap later with a formal router API. Right now it has no impact on the sidebar but we could make the sidebar routing part of this hook when we make the Navigator component "controllable".
| // Regular page | ||
| if ( path === '/page' ) { | ||
| const isListLayout = | ||
| isCustom !== 'true' && ( ! type || type === 'list' ); |
There was a problem hiding this comment.
This is the difficulty here, basically the route definition relies on the "type" of the dataview (layout), which means I had to add this information to the URL.
There's maybe another way to solve this but in trunk the "router" can't access this information because it's only kept in memory.
There was a problem hiding this comment.
Why do you have concerns about having the type as a URL param?
This makes me consider a more general point: why routing can't act as a pure function of the URL? Given a URL, I'm able to share it with other person who will see the same screen I'm seeing. This, of course, extends to things other than the view type (filters/search, pagination, etc.).
There was a problem hiding this comment.
I think the unknown here is that most of the views could either become entities in the future or be persisted in preferences or things like that, which means the "type" in the url redundant and something we'd want to ditch.
There was a problem hiding this comment.
Are we okay with showing the front page when we have no selection in pages list? Personally I think this a bit confusing..
There was a problem hiding this comment.
Are we okay with showing the front page when we have no selection in pages list? Personally I think this a bit confusing..
The way I see it, it's just how the (rest of the) site editor works, so it makes sense that pages does the same.
There was a problem hiding this comment.
I agree that it's not entirely clear whether the "home page" is the right choice there. A placeholder (select item kind of thing) could work like emails but I agree that for now it's probably fine. I think it would be good to show bulk actions there somehow in case of multi-selections... But all of these can be seen as follow-up improvements.
| @@ -300,3 +293,16 @@ | |||
| } | |||
|
|
|||
| } | |||
|
|
|||
| .edit-site-layout__area { | |||
There was a problem hiding this comment.
This is the style that make the area look like a rounded frame. This style maybe be shared with the "preview" as well.
| if ( isCustom === 'false' && type ) { | ||
| return { | ||
| ...defaultView, | ||
| type, |
There was a problem hiding this comment.
We override the default view with the "type" that comes from the URL.
| type: viewToSet.type, | ||
| } ); | ||
| } | ||
| setView( viewToSet ); |
There was a problem hiding this comment.
In addition to updating the view object, we also update the url if the "view type" changes. Maybe a better way is to split out the type from the view state.
| if ( isCustom === 'false' && view?.type === LAYOUT_LIST ) { | ||
| history.push( { | ||
| ...params, | ||
| postId: items.length === 1 ? items[ 0 ].id : undefined, |
There was a problem hiding this comment.
The selection change persists in the url which allow us to update the "preview" area with the right postId.
There was a problem hiding this comment.
This is nice. As a follow-up, we should consider updating the content area as well (update the selection).
There was a problem hiding this comment.
you mean making the selection in dataviews controlled right?
| onDetailsChange={ onDetailsChange } | ||
| /> | ||
| </Page> | ||
| { view.type === LAYOUT_LIST && ( |
There was a problem hiding this comment.
All what's after that has been removed, it now corresponds to the default "preview" area rendered in the Layout.
|
Size Change: +918 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
I'm starting to look into the PR and familiarizing myself with a few things. I've noticed that custom views don't seem to update the Gravacao.do.ecra.2024-01-18.as.10.45.50.mov |
|
@oandregal Yes, I did that on purpose, basically, the "list" view on custom views is just a full width list view without previews. I think that's fine for a start. We have options to address that, I just wanted to keep it simple for now. |
| } | ||
|
|
||
| // Regular other post types | ||
| if ( postType && postId ) { |
There was a problem hiding this comment.
This is the details view, as it exists today. As a follow-up, we'll need to update this to absorb the details as part of the content area (it's now part of the sidebar). (Mockup).
There was a problem hiding this comment.
Indeed, we might need to have it specific per post type. Remains to be seen.
| }; | ||
| } | ||
|
|
||
| // Fallback shows the home page preview |
There was a problem hiding this comment.
This is rendered when path is /wp_template, /wp_global_styles, /navigation.
There was a problem hiding this comment.
yes, and the home page of the site editor too.
oandregal
force-pushed
the
update/refactor-site-editor-router
branch
from
08bf870 to
0bdacb3
Compare
|
@oandregal maybe you missed a small change to |
| margin: $header-height 0 0; | ||
| border-radius: 0; | ||
| padding: 0; | ||
| overflow-x: auto; | ||
| min-height: 100%; |
There was a problem hiding this comment.
There is something wrong with the styles in patterns page with no experiment enabled and you can't scroll. I haven't found the culprit yet..
There was a problem hiding this comment.
Yes, I was wondering whether I should fix it or not, especially because we're stabilizing that page very soon. Just waiting for the "click on preview" to edit.
There was a problem hiding this comment.
The issue with scroll is larger: I cannot scroll any content area (patterns, templates, pages) with or without the experiment enabled. For example, this is how it works in trunk:
Gravacao.do.ecra.2024-01-18.as.18.49.05.mov
There was a problem hiding this comment.
Ok, I'll check this.
| <Editor isLoading={ isSiteEditorLoading } /> | ||
| ), | ||
| }, | ||
| widths: { |
There was a problem hiding this comment.
The concept of widths in a router 'API' is weird. I'm wondering if we could apply this in the content prop and have some css rules in edit-site-layout__area container like width: max-content or something(I haven't tried anything to see if it's possible with css).
There was a problem hiding this comment.
It's possible but the reason I kept it here for now is that I thought that maybe the "resizing" that is implemented in layout could use that width at some point.
There was a problem hiding this comment.
I think this is a good start and we can iterate. One important thing though is the comment for current patterns page(no scroll). With a quick follow up tomorrow, we can update the patterns page too I guess.
I'll approve, but since there are too many things to test(different views with and without the experiment), I'd appreciate some more testing from others too.
| @@ -154,7 +138,7 @@ export default function Layout() { | |||
| // Sets the right context for the command palette | |||
| let commandContext = 'site-editor'; | |||
|
|
|||
| if ( canvasMode === 'edit' && isEditorPage ) { | |||
There was a problem hiding this comment.
I looked around and haven't found any page with canvasMode=edit that wasn't in the editor.
There was a problem hiding this comment.
Also interacted with the command palette in various places, and it seems to work as expected.
| @@ -228,7 +212,7 @@ export default function Layout() { | |||
| /> | |||
|
|
|||
| <AnimatePresence initial={ false }> | |||
| { isEditorPage && isEditing && ( | |||
| { canvasMode === 'edit' && ( | |||
There was a problem hiding this comment.
Same as above: didn't find any page with canvasMode=edit that wasn't the editor.
There was a problem hiding this comment.
yes, that's the point here. The header is only shown when the canvasMode is edit, I guess your remark is a bit unclear to me.
There was a problem hiding this comment.
This wasn't meant as something you have to address but as something I've checked, and I see as correct in the PR. Sorry for the confusion :)
| @@ -272,7 +256,7 @@ export default function Layout() { | |||
| className="edit-site-layout__sidebar-region" | |||
| > | |||
| <AnimatePresence> | |||
| { showSidebar && ( | |||
| { canvasMode === 'view' && ( | |||
There was a problem hiding this comment.
This change makes the sidebar take the whole screen in small viewports for pages and templates, while before it showed the content area. Note that showing the sidebar is the same behavior that navigation, styles, and patterns have in trunk right now. I think it's fine to go with this. The previous logic based on listing/editor pages was hard to follow and created inconsistencies.
There was a problem hiding this comment.
I agree with this remark, I think the previous logic was too complex but I do believe it's better to show the list of "pages" and "templates" directly (not the sidebar), I believe we should try to follow-up with this. Maybe the router needs to be aware of viewport and change the areas accordingly.
There was a problem hiding this comment.
Yeah, agree. We should show the content area instead.
| ) } | ||
|
|
||
| { areas.preview && ( | ||
| <div className="edit-site-layout__canvas-container"> |
There was a problem hiding this comment.
I'd appreciate it if we could consolidate the classes around the concept of "areas" as well. These are being used in a few places, so it can be done in a follow-up as well.
There was a problem hiding this comment.
I agree, the problem is that the "preview" area is a bit complex with animations and stuff so I was afraid of touching it. Could be a follow-up too.
|
Edit 2: I can no longer reproduce this issue, either in
Gravacao.do.ecra.2024-01-18.as.18.18.48.mov |
| @@ -189,12 +195,25 @@ function useResolveEditedEntityAndContext( { postId, postType } ) { | |||
| return { postType, postId }; | |||
| } | |||
|
|
|||
| // Some URLs in list views are different | |||
There was a problem hiding this comment.
I'm a bit confused with the logic of this hook. Specifically as to why sometimes we set the context with postType/postId and other times we set postType/postId directly. This is a bit outside the scope of this PR, so I won't block it – though I'd welcome some pointer for me to learn up (and possibly ways to improve in follow-ups).
I've tested that the Pages and Templates with postId (list view with item selected) work fine.
There was a problem hiding this comment.
So the return of this hook gives the postType and postId that is going to be rendered in the "preview" and the "editor".
For "pages", that postType and postId is the "template" of the page, not the page directly because it's the root element. So in order to fill the template with the right postId, we provide it as "context" (BlockContext for the editor which ensures blocks like postContent block... will use the right postId).
This kind of relates with the unification efforts between post and site editors. I think ultimately, there are ways to simplify this further but there's some editSite state that forces us to keep this hook for now.
There was a problem hiding this comment.
That's helpful, thanks for the context.
| @@ -10,7 +10,6 @@ import { dateI18n, getDate, getSettings } from '@wordpress/date'; | |||
| import { privateApis as routerPrivateApis } from '@wordpress/router'; | |||
| import { useSelect, useDispatch } from '@wordpress/data'; | |||
| import { DataViews } from '@wordpress/dataviews'; | |||
| import { ENTER, SPACE } from '@wordpress/keycodes'; | |||
There was a problem hiding this comment.
I can no longer access the editor via the keyboard in any page. Once the preview is focused, the ENTER key doesn't trigger the edit canvas mode. SPACE triggers a scroll.
Gravacao.do.ecra.2024-01-18.as.18.55.17.mov
Interestingly, in trunk, this behavior was only working in dataviews-powered pages, as far as I've tested.
There was a problem hiding this comment.
I remember having to add an extra div to the PostPreview editor wrapper to make this work. Perhaps it was never considered before dataviews? If so, it could be a follow-up to add this behavior to the preview area in any page.
There was a problem hiding this comment.
The PostPreview has been replaced with the regular "Editor"/"Preview" that we use in the site editor for all the site editor pages basically. So there's a chance that the bug you're mentioning has always existed for the "frame" of the site editor.
@youknowriad @ntsekouras the scrolling issue is larger: it happens for any content area (cannot scroll any dataviews layouts, for example), with and without the experiment enabled. Given the time constraints, I'm fine if we want to merge this PR for us to start several follow-ups in parallel, but the issue is a bit severe. Other than that, I haven't found any blocker. The direction and changes are great 👏 |
|
@oandregal I agree we should fix the scrolling issue before merging. |
|
Ok I've fixed the scrolling behavior. |
|
Flaky tests detected in adf0454. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7585414116
|
|
This PR appears to have introduced some layout regressions. I have summarized what I discovered in #58044. |
Related #55083
What?
The List data views (either pages or templates) render two "areas" in the site editor:
In trunk, these two areas are rendered by the same component within the "frame". The problem with that approach is that we have other routes where the "preview" is still there but not the "content" area and we want seamless transitions and animations between all of these pages.
This means that we need to have separate the "preview" area from the "content" area and potentially keep the same "preview" area between different routes regardless of whether they use "list" layout or not.
This PR implements this and can also be considered a start of a more formal routing API (later down the road) in the site editor. Note that I didn't touch the sidebar on purpose, the fact that we're still relying on an uncontrolled "Navigator" component makes it hard to refactor as a "routing area".
Testing Instructions
1- Navigate in the site editor (everywhere :P) with both the "dataviews" experiment enabled and disabled.