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.

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

Fix browser history when synchronising state with urls #48731

Merged
merged 11 commits into from
Mar 6, 2023
4 changes: 3 additions & 1 deletion packages/edit-site/src/components/list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,10 @@ import useTitle from '../routes/use-title';

export default function List() {
const {
params: { postType: templateType },
params: { path },
} = useLocation();
const templateType =
path === '/wp_template/all' ? 'wp_template' : 'wp_template_part';

useRegisterShortcuts();

Expand Down
11 changes: 9 additions & 2 deletions packages/edit-site/src/components/routes/link.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { addQueryArgs } from '@wordpress/url';
import { addQueryArgs, getQueryArgs, removeQueryArgs } from '@wordpress/url';

/**
* Internal dependencies
Expand All @@ -21,8 +21,15 @@ export function useLink( params = {}, state, shouldReplace = false ) {
}
}

const currentArgs = getQueryArgs( window.location.href );
const currentUrlWithoutArgs = removeQueryArgs(
window.location.href,
...Object.keys( currentArgs )
);
const newUrl = addQueryArgs( currentUrlWithoutArgs, params );

return {
href: addQueryArgs( window.location.href, params ),
href: newUrl,
onClick,
};
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,12 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { useDispatch, useSelect } from '@wordpress/data';
import { useDispatch } from '@wordpress/data';
import {
__experimentalUseNavigator as useNavigator,
ExternalLink,
} from '@wordpress/components';
import { store as coreStore } from '@wordpress/core-data';
import { useEntityRecord } from '@wordpress/core-data';
import { decodeEntities } from '@wordpress/html-entities';
import { pencil } from '@wordpress/icons';

Expand All @@ -24,26 +24,11 @@ export default function SidebarNavigationScreenNavigationItem() {
const {
params: { postType, postId },
} = useNavigator();

const { post } = useSelect(
( select ) => {
const { getEntityRecord } = select( coreStore );

// The currently selected entity to display.
// Typically template or template part in the site editor.
return {
post:
postId && postType
? getEntityRecord( 'postType', postType, postId )
: null,
};
},
[ postType, postId ]
);
const { record } = useEntityRecord( 'postType', postType, postId );

return (
<SidebarNavigationScreen
title={ post ? decodeEntities( post?.title?.rendered ) : null }
title={ record ? decodeEntities( record?.title?.rendered ) : null }
actions={
<SidebarButton
onClick={ () => setCanvasMode( 'edit' ) }
Expand All @@ -53,16 +38,16 @@ export default function SidebarNavigationScreenNavigationItem() {
}
content={
<>
{ post?.link ? (
{ record?.link ? (
<ExternalLink
className="edit-site-sidebar-navigation-screen__page-link"
href={ post.link }
href={ record.link }
>
{ post.link }
{ record.link }
</ExternalLink>
) : null }
{ post
? decodeEntities( post?.description?.rendered )
{ record
? decodeEntities( record?.description?.rendered )
: null }
</>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { __ } from '@wordpress/i18n';
import { useDispatch } from '@wordpress/data';
import { pencil } from '@wordpress/icons';
import { __experimentalUseNavigator as useNavigator } from '@wordpress/components';

/**
* Internal dependencies
Expand All @@ -15,8 +16,13 @@ import { store as editSiteStore } from '../../store';
import SidebarButton from '../sidebar-button';

export default function SidebarNavigationScreenTemplate() {
const { params } = useNavigator();
const { postType, postId } = params;
const { setCanvasMode } = unlock( useDispatch( editSiteStore ) );
const { getDescription, getTitle, record } = useEditedEntityRecord();
const { getDescription, getTitle, record } = useEditedEntityRecord(
postType,
postId
);
let description = getDescription();
if ( ! description && record.is_custom ) {
description = __(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,52 @@ export default function useSyncCanvasModeWithURL() {
);
const { setCanvasMode } = unlock( useDispatch( editSiteStore ) );
const currentCanvasMode = useRef( canvasMode );
const { canvas: canvasInUrl = 'view' } = params;
const { canvas: canvasInUrl } = params;
const currentCanvasInUrl = useRef( canvasInUrl );
const currentUrlParams = useRef( params );
useEffect( () => {
currentUrlParams.current = params;
}, [ params ] );

useEffect( () => {
currentCanvasMode.current = canvasMode;
if ( currentCanvasMode !== currentCanvasInUrl ) {
if ( canvasMode === 'init' ) {
return;
}

if (
canvasMode === 'edit' &&
currentCanvasInUrl.current !== canvasMode
) {
history.push( {
...currentUrlParams.current,
canvas: 'edit',
} );
}

if (
canvasMode === 'view' &&
currentCanvasInUrl.current !== undefined
) {
history.push( {
...params,
canvas: canvasMode,
...currentUrlParams.current,
canvas: undefined,
ntsekouras marked this conversation as resolved.
Show resolved Hide resolved
} );
}
}, [ canvasMode ] );
}, [ canvasMode, history ] );

useEffect( () => {
currentCanvasInUrl.current = canvasInUrl;
if ( canvasInUrl !== currentCanvasMode.current ) {
setCanvasMode( canvasInUrl );
if (
canvasInUrl === undefined &&
currentCanvasMode.current !== 'view'
) {
setCanvasMode( 'view' );
} else if (
canvasInUrl === 'edit' &&
currentCanvasMode.current !== 'edit'
) {
setCanvasMode( 'edit' );
}
}, [ canvasInUrl ] );
}, [ canvasInUrl, setCanvasMode ] );
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,16 @@ export default function useSyncPathWithURL() {
} = useNavigator();
const currentUrlParams = useRef( urlParams );
const currentPath = useRef( navigatorLocation.path );
const isMounting = useRef( true );

useEffect( () => {
// The navigatorParams are only initially filled properly when the
// navigator screens mount. so we ignore the first synchronisation.
if ( isMounting.current ) {
isMounting.current = false;
return;
}

function updateUrlParams( newUrlParams ) {
if (
Object.entries( newUrlParams ).every( ( [ key, value ] ) => {
Expand All @@ -65,17 +73,14 @@ export default function useSyncPathWithURL() {
postId: navigatorParams?.postId,
path: undefined,
} );
} else if ( navigatorParams?.postType && ! navigatorParams?.postId ) {
updateUrlParams( {
postType: navigatorParams?.postType,
path: navigatorLocation.path,
postId: undefined,
} );
} else {
updateUrlParams( {
postType: undefined,
postId: undefined,
path: navigatorLocation.path,
path:
navigatorLocation.path === '/'
? undefined
: navigatorLocation.path,
} );
}
}, [ navigatorLocation?.path, navigatorParams, history ] );
Expand Down
3 changes: 0 additions & 3 deletions packages/edit-site/src/components/template-details/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,6 @@ export default function TemplateDetails( { template, onClose } ) {

// TODO: We should update this to filter by template part's areas as well.
const browseAllLinkProps = useLink( {
canvas: 'view',
Copy link
Member

Choose a reason for hiding this comment

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

I think this causes regression of #48301.

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 should be "undefined" now because "view" is the default value. But I guess it suffers from the issue I talk about here #48731 (comment)

postType: template.type,
postId: undefined,
path: '/' + template.type + '/all',
} );

Expand Down
44 changes: 26 additions & 18 deletions packages/edit-site/src/components/use-edited-entity-record/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,25 +11,33 @@ import { decodeEntities } from '@wordpress/html-entities';
*/
import { store as editSiteStore } from '../../store';

export default function useEditedEntityRecord() {
const { record, title, description, isLoaded } = useSelect( ( select ) => {
const { getEditedPostType, getEditedPostId } = select( editSiteStore );
const { getEditedEntityRecord } = select( coreStore );
const { __experimentalGetTemplateInfo: getTemplateInfo } =
select( editorStore );
const postType = getEditedPostType();
const postId = getEditedPostId();
const _record = getEditedEntityRecord( 'postType', postType, postId );
const _isLoaded = !! postId;
const templateInfo = getTemplateInfo( _record );
export default function useEditedEntityRecord( postType, postId ) {
const { record, title, description, isLoaded } = useSelect(
( select ) => {
const { getEditedPostType, getEditedPostId } =
select( editSiteStore );
const { getEditedEntityRecord } = select( coreStore );
const { __experimentalGetTemplateInfo: getTemplateInfo } =
select( editorStore );
const usedPostType = postType ?? getEditedPostType();
const usedPostId = postId ?? getEditedPostId();
const _record = getEditedEntityRecord(
'postType',
usedPostType,
usedPostId
);
const _isLoaded = !! usedPostId;
const templateInfo = getTemplateInfo( _record );

return {
record: _record,
title: templateInfo.title,
description: templateInfo.description,
isLoaded: _isLoaded,
};
}, [] );
return {
record: _record,
title: templateInfo.title,
description: templateInfo.description,
isLoaded: _isLoaded,
};
},
[ postType, postId ]
);

return {
isLoaded,
Expand Down
22 changes: 13 additions & 9 deletions packages/edit-site/src/utils/history.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,27 +6,31 @@ import { createBrowserHistory } from 'history';
/**
* WordPress dependencies
*/
import { addQueryArgs } from '@wordpress/url';
import { addQueryArgs, getQueryArgs, removeQueryArgs } from '@wordpress/url';

const history = createBrowserHistory();

const originalHistoryPush = history.push;
const originalHistoryReplace = history.replace;

function push( params, state ) {
return originalHistoryPush.call(
history,
addQueryArgs( window.location.href, params ),
state
const currentArgs = getQueryArgs( window.location.href );
const currentUrlWithoutArgs = removeQueryArgs(
window.location.href,
...Object.keys( currentArgs )
);
const newUrl = addQueryArgs( currentUrlWithoutArgs, params );
return originalHistoryPush.call( history, newUrl, state );
}

function replace( params, state ) {
return originalHistoryReplace.call(
history,
addQueryArgs( window.location.href, params ),
state
const currentArgs = getQueryArgs( window.location.href );
const currentUrlWithoutArgs = removeQueryArgs(
window.location.href,
...Object.keys( currentArgs )
);
const newUrl = addQueryArgs( currentUrlWithoutArgs, params );
return originalHistoryReplace.call( history, newUrl, state );
}

history.push = push;
Expand Down
60 changes: 60 additions & 0 deletions test/e2e/specs/site-editor/browser-history.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/**
* WordPress dependencies
*/
const { test, expect } = require( '@wordpress/e2e-test-utils-playwright' );

test.describe( 'Site editor browser history', () => {
test.beforeAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'emptytheme' );
} );

test.afterAll( async ( { requestUtils } ) => {
await requestUtils.activateTheme( 'twentytwentyone' );
} );

test( 'Back button works properly', async ( { admin, page } ) => {
await admin.visitAdminPage( 'index.php' );
await admin.visitSiteEditor();
await expect( page ).toHaveURL( '/wp-admin/site-editor.php' );

// Navigate to a single template
await page.click( 'role=button[name="Templates"]' );
await page.click( 'role=button[name="Index"]' );
await expect( page ).toHaveURL(
'/wp-admin/site-editor.php?postType=wp_template&postId=emptytheme%2F%2Findex'
);

// Navigate back to the template list
await page.goBack();
await expect( page ).toHaveURL(
'/wp-admin/site-editor.php?path=%2Fwp_template'
);

// Navigate back to the dashboard
await page.goBack();
await page.goBack();
await expect( page ).toHaveURL( '/wp-admin/index.php' );
} );

test( 'Opens the template list from the template details view', async ( {
admin,
page,
} ) => {
await admin.visitSiteEditor( {
postType: 'wp_template',
postId: 'emptytheme//index',
canvas: 'edit',
} );

// Navigate to the template list
await page.click( 'role=button[name="Show template details"]' );
await page.click( 'role=link[name="Manage all templates"]' );

await expect( page ).toHaveURL(
'/wp-admin/site-editor.php?path=%2Fwp_template%2Fall'
);

const title = page.getByRole( 'heading', { level: 1 } );
await expect( title ).toHaveText( 'Templates' );
} );
} );