Skip to content
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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jan 19, 2024
Merged

Site Editor: Initial routing refactoring to separate preview from list view #57938

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
65f84fe
Site Editor: Initial routing refactoring to separate preview from lis…
youknowriad Jan 17, 2024
28354ad
Remove duplicated class
youknowriad Jan 18, 2024
0bdacb3
Update router for template root
oandregal Jan 18, 2024
f80dd82
Fix selected postId when selecting templates in list view
youknowriad Jan 18, 2024
a34452f
Small tweaks to the routing
youknowriad Jan 18, 2024
e44a1b7
Rename type to layout in the url
youknowriad Jan 18, 2024
adf0454
Fix the scrolling behavior
youknowriad Jan 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
176 changes: 80 additions & 96 deletions packages/edit-site/src/components/layout/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand All @@ -154,7 +138,7 @@ export default function Layout() {
// Sets the right context for the command palette
let commandContext = 'site-editor';

if ( canvasMode === 'edit' && isEditorPage ) {
Copy link
Member

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.

Copy link
Member

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.

if ( canvasMode === 'edit' ) {
commandContext = 'site-editor-edit';
}
if ( hasBlockSelected ) {
Expand Down Expand Up @@ -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,
}
Expand Down Expand Up @@ -228,7 +212,7 @@ export default function Layout() {
/>

<AnimatePresence initial={ false }>
{ isEditorPage && isEditing && (
{ canvasMode === 'edit' && (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above: didn't find any page with canvasMode=edit that wasn't the editor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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"
Expand Down Expand Up @@ -272,7 +256,7 @@ export default function Layout() {
className="edit-site-layout__sidebar-region"
>
<AnimatePresence>
{ showSidebar && (
{ canvasMode === 'view' && (
Copy link
Member

Choose a reason for hiding this comment

The 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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 } }
Expand All @@ -296,82 +280,82 @@ export default function Layout() {

<SavePanel />

{ showCanvas && (
<>
{ isListPage && <PageMain /> }
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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' && (
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the new "content" area.

Copy link
Member

Choose a reason for hiding this comment

The 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 canvasMode shouldn't be absorbed by the useLayout hook?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The 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>
    );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<div
className="edit-site-layout__area"
style={ {
maxWidth: widths?.content,
} }
>
{ areas.content }
</div>
) }

{ areas.preview && (
<div className="edit-site-layout__canvas-container">
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>
Expand Down
103 changes: 103 additions & 0 deletions packages/edit-site/src/components/layout/router.js
View file
Open in desktop
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() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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, layout, isCustom } = params ?? {};

// Regular page
if ( path === '/page' ) {
const isListLayout =
isCustom !== 'true' && ( ! layout || layout === 'list' );
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 ) {
Copy link
Member

Choose a reason for hiding this comment

The 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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' && ( ! layout || layout === 'list' );
return {
areas: {
content: <PageTemplates />,
preview: isListLayout && (
<Editor isLoading={ isSiteEditorLoading } />
),
},
widths: {
Copy link
Contributor

@ntsekouras ntsekouras Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is rendered when path is /wp_template, /wp_global_styles, /navigation.

Copy link
Contributor Author

@youknowriad youknowriad Jan 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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 } /> },
};
}
Loading
Loading