-
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: Initial routing refactoring to separate preview from list view #57938
Changes from 4 commits
65f84fe
28354ad
0bdacb3
f80dd82
a34452f
e44a1b7
adf0454
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 |
---|---|---|
|
@@ -31,17 +31,14 @@ import { | |
useBlockCommands, | ||
store as blockEditorStore, | ||
} from '@wordpress/block-editor'; | ||
import { privateApis as routerPrivateApis } from '@wordpress/router'; | ||
import { privateApis as coreCommandsPrivateApis } from '@wordpress/core-commands'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import Sidebar from '../sidebar'; | ||
import Editor from '../editor'; | ||
import ErrorBoundary from '../error-boundary'; | ||
import { store as editSiteStore } from '../../store'; | ||
import getIsListPage from '../../utils/get-is-list-page'; | ||
import Header from '../header-edit-mode'; | ||
import useInitEditedEntityFromURL from '../sync-state-with-url/use-init-edited-entity-from-url'; | ||
import SiteHub from '../site-hub'; | ||
|
@@ -53,12 +50,11 @@ import KeyboardShortcutsRegister from '../keyboard-shortcuts/register'; | |
import KeyboardShortcutsGlobal from '../keyboard-shortcuts/global'; | ||
import { useCommonCommands } from '../../hooks/commands/use-common-commands'; | ||
import { useEditModeCommands } from '../../hooks/commands/use-edit-mode-commands'; | ||
import PageMain from '../page-main'; | ||
import { useIsSiteEditorLoading } from './hooks'; | ||
import useLayoutAreas from './router'; | ||
|
||
const { useCommands } = unlock( coreCommandsPrivateApis ); | ||
const { useCommandContext } = unlock( commandsPrivateApis ); | ||
const { useLocation } = unlock( routerPrivateApis ); | ||
const { useGlobalStyle } = unlock( blockEditorPrivateApis ); | ||
|
||
const ANIMATION_DURATION = 0.5; | ||
|
@@ -72,10 +68,7 @@ export default function Layout() { | |
useCommonCommands(); | ||
useBlockCommands(); | ||
|
||
const { params } = useLocation(); | ||
const isMobileViewport = useViewportMatch( 'medium', '<' ); | ||
const isListPage = getIsListPage( params, isMobileViewport ); | ||
const isEditorPage = ! isListPage; | ||
|
||
const { | ||
isDistractionFree, | ||
|
@@ -109,26 +102,17 @@ export default function Layout() { | |
select( blockEditorStore ).getBlockSelectionStart(), | ||
}; | ||
}, [] ); | ||
const isEditing = canvasMode === 'edit'; | ||
const navigateRegionsProps = useNavigateRegions( { | ||
previous: previousShortcut, | ||
next: nextShortcut, | ||
} ); | ||
const disableMotion = useReducedMotion(); | ||
const showSidebar = | ||
( isMobileViewport && canvasMode === 'view' && ! isListPage ) || | ||
( ! isMobileViewport && ( canvasMode === 'view' || ! isEditorPage ) ); | ||
const showCanvas = | ||
( isMobileViewport && isEditorPage && isEditing ) || | ||
! isMobileViewport || | ||
! isEditorPage; | ||
const isFullCanvas = | ||
( isMobileViewport && isListPage ) || ( isEditorPage && isEditing ); | ||
const [ canvasResizer, canvasSize ] = useResizeObserver(); | ||
const [ fullResizer ] = useResizeObserver(); | ||
const isEditorLoading = useIsSiteEditorLoading(); | ||
const [ isResizableFrameOversized, setIsResizableFrameOversized ] = | ||
useState( false ); | ||
const { areas, widths } = useLayoutAreas(); | ||
|
||
// This determines which animation variant should apply to the header. | ||
// There is also a `isDistractionFreeHovering` state that gets priority | ||
|
@@ -154,7 +138,7 @@ export default function Layout() { | |
// Sets the right context for the command palette | ||
let commandContext = 'site-editor'; | ||
|
||
if ( canvasMode === 'edit' && isEditorPage ) { | ||
if ( canvasMode === 'edit' ) { | ||
commandContext = 'site-editor-edit'; | ||
} | ||
if ( hasBlockSelected ) { | ||
|
@@ -185,9 +169,9 @@ export default function Layout() { | |
'edit-site-layout', | ||
navigateRegionsProps.className, | ||
{ | ||
'is-distraction-free': isDistractionFree && isEditing, | ||
'is-full-canvas': isFullCanvas, | ||
'is-edit-mode': isEditing, | ||
'is-distraction-free': | ||
isDistractionFree && canvasMode === 'edit', | ||
'is-full-canvas': canvasMode === 'edit', | ||
'has-fixed-toolbar': hasFixedToolbar, | ||
'is-block-toolbar-visible': hasBlockSelected, | ||
} | ||
|
@@ -228,7 +212,7 @@ export default function Layout() { | |
/> | ||
|
||
<AnimatePresence initial={ false }> | ||
{ isEditorPage && isEditing && ( | ||
{ canvasMode === 'edit' && ( | ||
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. Same as above: didn't find any page with 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :) 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. I'm so dumb :P |
||
<NavigableRegion | ||
key="header" | ||
className="edit-site-layout__header" | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, agree. We should show the content area instead. |
||
<motion.div | ||
initial={ { opacity: 0 } } | ||
animate={ { opacity: 1 } } | ||
|
@@ -296,82 +280,82 @@ export default function Layout() { | |
|
||
<SavePanel /> | ||
|
||
{ showCanvas && ( | ||
<> | ||
{ isListPage && <PageMain /> } | ||
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. The list page is now rendered within the "content" area while the editor is rendered within the "preview" area. |
||
{ isEditorPage && ( | ||
<div className="edit-site-layout__canvas-container"> | ||
{ canvasResizer } | ||
{ !! canvasSize.width && ( | ||
<motion.div | ||
whileHover={ | ||
isEditorPage && | ||
canvasMode === 'view' | ||
? { | ||
scale: 1.005, | ||
transition: { | ||
duration: | ||
disableMotion | ||
? 0 | ||
: 0.5, | ||
ease: 'easeOut', | ||
}, | ||
} | ||
: {} | ||
{ areas.content && canvasMode !== 'edit' && ( | ||
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 the new "content" area. 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. 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 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.
Can you clarify a bit more what you have in mind? 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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree that's the goal.
|
||
<div | ||
className="edit-site-layout__area" | ||
style={ { | ||
maxWidth: widths?.content, | ||
} } | ||
> | ||
{ areas.content } | ||
</div> | ||
) } | ||
|
||
{ areas.preview && ( | ||
<div className="edit-site-layout__canvas-container"> | ||
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. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{ canvasResizer } | ||
{ !! canvasSize.width && ( | ||
<motion.div | ||
whileHover={ | ||
canvasMode === 'view' | ||
? { | ||
scale: 1.005, | ||
transition: { | ||
duration: disableMotion | ||
? 0 | ||
: 0.5, | ||
ease: 'easeOut', | ||
}, | ||
} | ||
: {} | ||
} | ||
initial={ false } | ||
layout="position" | ||
className={ classnames( | ||
'edit-site-layout__canvas', | ||
{ | ||
'is-right-aligned': | ||
isResizableFrameOversized, | ||
} | ||
) } | ||
transition={ { | ||
type: 'tween', | ||
duration: disableMotion | ||
? 0 | ||
: ANIMATION_DURATION, | ||
ease: 'easeOut', | ||
} } | ||
> | ||
<ErrorBoundary> | ||
<ResizableFrame | ||
isReady={ ! isEditorLoading } | ||
isFullWidth={ | ||
canvasMode === 'edit' | ||
} | ||
initial={ false } | ||
layout="position" | ||
className={ classnames( | ||
'edit-site-layout__canvas', | ||
{ | ||
'is-right-aligned': | ||
isResizableFrameOversized, | ||
} | ||
) } | ||
transition={ { | ||
type: 'tween', | ||
duration: disableMotion | ||
? 0 | ||
: ANIMATION_DURATION, | ||
ease: 'easeOut', | ||
defaultSize={ { | ||
width: | ||
canvasSize.width - | ||
24 /* $canvas-padding */, | ||
height: canvasSize.height, | ||
} } | ||
isOversized={ | ||
isResizableFrameOversized | ||
} | ||
setIsOversized={ | ||
setIsResizableFrameOversized | ||
} | ||
innerContentStyle={ { | ||
background: | ||
gradientValue ?? | ||
backgroundColor, | ||
} } | ||
> | ||
<ErrorBoundary> | ||
<ResizableFrame | ||
isReady={ | ||
! isEditorLoading | ||
} | ||
isFullWidth={ isEditing } | ||
defaultSize={ { | ||
width: | ||
canvasSize.width - | ||
24 /* $canvas-padding */, | ||
height: canvasSize.height, | ||
} } | ||
isOversized={ | ||
isResizableFrameOversized | ||
} | ||
setIsOversized={ | ||
setIsResizableFrameOversized | ||
} | ||
innerContentStyle={ { | ||
background: | ||
gradientValue ?? | ||
backgroundColor, | ||
} } | ||
> | ||
<Editor | ||
isLoading={ | ||
isEditorLoading | ||
} | ||
/> | ||
</ResizableFrame> | ||
</ErrorBoundary> | ||
</motion.div> | ||
) } | ||
</div> | ||
{ areas.preview } | ||
</ResizableFrame> | ||
</ErrorBoundary> | ||
</motion.div> | ||
) } | ||
</> | ||
</div> | ||
) } | ||
</div> | ||
</div> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,103 @@ | ||
/** | ||
* WordPress dependencies | ||
*/ | ||
import { privateApis as routerPrivateApis } from '@wordpress/router'; | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
import { unlock } from '../../lock-unlock'; | ||
import { useIsSiteEditorLoading } from './hooks'; | ||
import Editor from '../editor'; | ||
import DataviewsPatterns from '../page-patterns/dataviews-patterns'; | ||
import PagePages from '../page-pages'; | ||
import PagePatterns from '../page-patterns'; | ||
import PageTemplateParts from '../page-template-parts'; | ||
import PageTemplates from '../page-templates'; | ||
|
||
const { useLocation } = unlock( routerPrivateApis ); | ||
|
||
export default function useLayoutAreas() { | ||
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 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". |
||
const isSiteEditorLoading = useIsSiteEditorLoading(); | ||
const { params } = useLocation(); | ||
const { postType, postId, path, type, isCustom } = params ?? {}; | ||
|
||
// Regular page | ||
if ( path === '/page' ) { | ||
const isListLayout = | ||
isCustom !== 'true' && ( ! type || type === 'list' ); | ||
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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you have concerns about having the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we okay with showing the front page when we have no selection in 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.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
return { | ||
areas: { | ||
content: window.__experimentalAdminViews ? ( | ||
<PagePages /> | ||
) : undefined, | ||
preview: ( isListLayout || | ||
! window.__experimentalAdminViews ) && ( | ||
<Editor isLoading={ isSiteEditorLoading } /> | ||
), | ||
}, | ||
widths: { | ||
content: | ||
window.__experimentalAdminViews && isListLayout | ||
? 380 | ||
: undefined, | ||
}, | ||
}; | ||
} | ||
|
||
// Regular other post types | ||
if ( postType && postId ) { | ||
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 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Indeed, we might need to have it specific per post type. Remains to be seen. |
||
return { | ||
areas: { | ||
preview: <Editor isLoading={ isSiteEditorLoading } />, | ||
}, | ||
}; | ||
} | ||
|
||
// Templates | ||
if ( | ||
path === '/wp_template/all' || | ||
( path === '/wp_template' && window?.__experimentalAdminViews ) | ||
) { | ||
const isListLayout = | ||
isCustom !== 'true' && ( ! type || type === 'list' ); | ||
return { | ||
areas: { | ||
content: <PageTemplates />, | ||
preview: isListLayout && ( | ||
<Editor isLoading={ isSiteEditorLoading } /> | ||
), | ||
}, | ||
widths: { | ||
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. The concept of widths in a router 'API' is weird. I'm wondering if we could apply this in the 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. 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. |
||
content: isListLayout ? 380 : undefined, | ||
}, | ||
}; | ||
} | ||
|
||
// Template parts | ||
if ( path === '/wp_template_part/all' ) { | ||
return { | ||
areas: { | ||
content: <PageTemplateParts />, | ||
}, | ||
}; | ||
} | ||
|
||
// Patterns | ||
if ( path === '/patterns' ) { | ||
return { | ||
areas: { | ||
content: window?.__experimentalAdminViews ? ( | ||
<DataviewsPatterns /> | ||
) : ( | ||
<PagePatterns /> | ||
), | ||
}, | ||
}; | ||
} | ||
|
||
// Fallback shows the home page preview | ||
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 rendered when path is 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. yes, and the home page of the site editor too. |
||
return { | ||
areas: { preview: <Editor isLoading={ isSiteEditorLoading } /> }, | ||
}; | ||
} |
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 looked around and haven't found any page with
canvasMode=edit
that wasn't in the editor.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.
Also interacted with the command palette in various places, and it seems to work as expected.