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

Improve guidance to editing template when focused on editing a page #51366

Merged
merged 11 commits into from
Jun 19, 2023
41 changes: 16 additions & 25 deletions packages/edit-site/src/components/block-editor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,7 @@ import ResizableEditor from './resizable-editor';
import EditorCanvas from './editor-canvas';
import { unlock } from '../../lock-unlock';
import EditorCanvasContainer from '../editor-canvas-container';
import {
DisableNonPageContentBlocks,
usePageContentFocusNotifications,
} from '../page-content-focus';
import PageContentFocusManager from '../page-content-focus-manager';

const { ExperimentalBlockEditorProvider } = unlock( blockEditorPrivateApis );

Expand All @@ -53,25 +50,20 @@ const LAYOUT = {

export default function BlockEditor() {
const { setIsInserterOpened } = useDispatch( editSiteStore );
const { storedSettings, templateType, canvasMode, hasPageContentFocus } =
useSelect(
( select ) => {
const {
getSettings,
getEditedPostType,
getCanvasMode,
hasPageContentFocus: _hasPageContentFocus,
} = unlock( select( editSiteStore ) );

return {
storedSettings: getSettings( setIsInserterOpened ),
templateType: getEditedPostType(),
canvasMode: getCanvasMode(),
hasPageContentFocus: _hasPageContentFocus(),
};
},
[ setIsInserterOpened ]
);
const { storedSettings, templateType, canvasMode } = useSelect(
( select ) => {
const { getSettings, getEditedPostType, getCanvasMode } = unlock(
select( editSiteStore )
);

return {
storedSettings: getSettings( setIsInserterOpened ),
templateType: getEditedPostType(),
canvasMode: getCanvasMode(),
};
},
[ setIsInserterOpened ]
);

const settingsBlockPatterns =
storedSettings.__experimentalAdditionalBlockPatterns ?? // WP 6.0
Expand Down Expand Up @@ -146,7 +138,6 @@ export default function BlockEditor() {
contentRef,
useClipboardHandler(),
useTypingObserver(),
usePageContentFocusNotifications(),
] );
const isMobileViewport = useViewportMatch( 'small', '<' );
const { clearSelectedBlock } = useDispatch( blockEditorStore );
Expand All @@ -172,7 +163,7 @@ export default function BlockEditor() {
onChange={ onChange }
useSubRegistry={ false }
>
{ hasPageContentFocus && <DisableNonPageContentBlocks /> }
<PageContentFocusManager contentRef={ contentRef } />
<TemplatePartConverter />
<SidebarInspectorFill>
<BlockInspector />
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { store as noticesStore } from '@wordpress/notices';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../store';

/**
* Component that displays a 'You are editing a template' notification when the
* user switches from focusing on editing page content to editing a template.
*/
export default function BackToPageNotification() {
useBackToPageNotification();
return null;
}

/**
* Hook that displays a 'You are editing a template' notification when the user
* switches from focusing on editing page content to editing a template.
*/
export function useBackToPageNotification() {
const hasPageContentFocus = useSelect(
( select ) => select( editSiteStore ).hasPageContentFocus(),
[]
);

const alreadySeen = useRef( false );
const prevHasPageContentFocus = useRef( false );

const { createInfoNotice } = useDispatch( noticesStore );
const { setHasPageContentFocus } = useDispatch( editSiteStore );

useEffect( () => {
if (
! alreadySeen.current &&
prevHasPageContentFocus.current &&
! hasPageContentFocus
) {
createInfoNotice( __( 'You are editing a template' ), {
isDismissible: true,
type: 'snackbar',
actions: [
{
label: __( 'Back to page' ),
onClick: () => setHasPageContentFocus( true ),
},
],
} );
alreadySeen.current = true;
}
prevHasPageContentFocus.current = hasPageContentFocus;
}, [
alreadySeen,
prevHasPageContentFocus,
Comment on lines +59 to +60
Copy link
Member

@Mamaduka Mamaduka Jun 19, 2023

Choose a reason for hiding this comment

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

Nit: There's no need to add mutable values like refs as dependencies. When you provide them, alreadySeen.current ESLint shows a warning.

hasPageContentFocus,
createInfoNotice,
setHasPageContentFocus,
] );
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const { useBlockEditingMode } = unlock( blockEditorPrivateApis );
* Component that when rendered, makes it so that the site editor allows only
* page content to be edited.
*/
export function DisableNonPageContentBlocks() {
export default function DisableNonPageContentBlocks() {
useDisableNonPageContentBlocks();
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
/**
* WordPress dependencies
*/
import { useDispatch } from '@wordpress/data';
import { useEffect, useState } from '@wordpress/element';
import { __ } from '@wordpress/i18n';
import { __experimentalConfirmDialog as ConfirmDialog } from '@wordpress/components';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../store';

/**
* Component that displays a 'Edit your template to edit this block'
* notification when the user is focusing on editing page content and clicks on
* a disabled template block.
*
* @param {Object} props
* @param {import('react').RefObject<HTMLElement>} props.contentRef Ref to the block
* editor iframe canvas.
*/
export default function EditTemplateDialog( { contentRef } ) {
const [ isOpen, setIsOpen ] = useState( false );

useEffect( () => {
const handleDblClick = ( event ) => {
if ( event.target.classList.contains( 'is-root-container' ) ) {
setIsOpen( true );
}
};
const canvas = contentRef.current;
canvas?.addEventListener( 'dblclick', handleDblClick );
return () => canvas?.removeEventListener( 'dblclick', handleDblClick );
}, [ contentRef.current ] );

const { setHasPageContentFocus } = useDispatch( editSiteStore );

return (
<ConfirmDialog
isOpen={ isOpen }
confirmButtonText={ __( 'Edit template' ) }
onConfirm={ () => {
setIsOpen( false );
setHasPageContentFocus( false );
} }
onCancel={ () => setIsOpen( false ) }
>
{ __( 'Edit your template to edit this block' ) }
Copy link
Member

Choose a reason for hiding this comment

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

Not a biggie for this PR, but this text can be misleading since you can click anywhere outside a content block to trigger the notice + modal, even on the latters' padding or margin.

2023-06-16 14 11 54

Maybe "Edit your template to edit this area" ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny nit: for text within the dialog, should we end the sentence in a full-stop? Also, not a blocker (and wording can be tweaked in follow-ups), but since we have a modal, I was wondering if it's worth including more explanatory text in this one. E.g. something along the lines of, "You are currently editing ${ pageTitle }, do you wish to switch to template editing?"... though now that I write that out, I'm not sure how much more helpful that is. Writing this sort of micro copy is hard! 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Will add the periods! 👍

Not 100% sure about changing the copy. Wdyt @SaxonF?

Copy link
Contributor

Choose a reason for hiding this comment

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

this sort of copy is indeed difficult. I'd be fine shipping as is and follow up with a copy pass.

</ConfirmDialog>
);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch } from '@wordpress/data';
import { useEffect, useRef } from '@wordpress/element';
import { store as noticesStore } from '@wordpress/notices';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../store';

/**
* Component that displays a 'Edit your template to edit this block'
* notification when the user is focusing on editing page content and clicks on
* a disabled template block.
*
* @param {Object} props
* @param {import('react').RefObject<HTMLElement>} props.contentRef Ref to the block
* editor iframe canvas.
*/
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
export default function EditTemplateNotification( { contentRef } ) {
useEditTemplateNotification( contentRef );
return null;
}

/**
* Hook that displays a 'Edit your template to edit this block' notification
* when the user is focusing on editing page content and clicks on a disabled
* template block.
*
* @param {import('react').RefObject<HTMLElement>} contentRef Ref to the block
* editor iframe canvas.
*/
function useEditTemplateNotification( contentRef ) {
const hasPageContentFocus = useSelect(
( select ) => select( editSiteStore ).hasPageContentFocus(),
[]
);
const { getNotices } = useSelect( noticesStore );

const { createInfoNotice } = useDispatch( noticesStore );
const { setHasPageContentFocus } = useDispatch( editSiteStore );

const lastNoticeId = useRef( 0 );

useEffect( () => {
const handleClick = async ( event ) => {
const isNoticeAlreadyShowing = getNotices().some(
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
( notice ) => notice.id === lastNoticeId.current
);
if (
! isNoticeAlreadyShowing &&
hasPageContentFocus &&
event.target.classList.contains( 'is-root-container' )
) {
const { notice } = await createInfoNotice(
__( 'Edit your template to edit this block' ),
{
isDismissible: true,
type: 'snackbar',
actions: [
{
label: __( 'Edit template' ),
onClick: () => setHasPageContentFocus( false ),
},
],
}
);
lastNoticeId.current = notice.id;
}
};
const canvas = contentRef.current;
canvas?.addEventListener( 'click', handleClick );
return () => canvas?.removeEventListener( 'click', handleClick );
}, [ lastNoticeId, hasPageContentFocus, contentRef.current ] );
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import { store as editSiteStore } from '../../store';
import DisableNonPageContentBlocks from './disable-non-page-content-blocks';
import EditTemplateDialog from './edit-template-dialog';
import EditTemplateNotification from './edit-template-notification';
import BackToPageNotification from './back-to-page-notification';

export default function PageContentFocusManager( { contentRef } ) {
const hasPageContentFocus = useSelect(
( select ) => select( editSiteStore ).hasPageContentFocus(),
[]
);
return (
<>
{ hasPageContentFocus && (
<>
<DisableNonPageContentBlocks />
<EditTemplateDialog contentRef={ contentRef } />
Copy link
Member

Choose a reason for hiding this comment

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

A nice follow up, if possible, would be to prevent <EditTemplateNotification /> from rendering when the dialog is open.

Screenshot 2023-06-16 at 1 28 08 pm

Copy link
Member Author

Choose a reason for hiding this comment

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

By putting the notification behind a timeout? Not sure how this could be done 😀

Copy link
Member

Choose a reason for hiding this comment

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

No idea either, but the double notification isn't ideal.

Not sure how to do it either without probably a refactor.

Just noting it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah maybe we can dismiss the notification automatically when modal appears. I'd have to smoosh the components together though. I guess that's not so bad.

Copy link
Member Author

Choose a reason for hiding this comment

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

How's this? f7ff0dc

Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

2023-06-19.14.26.06.mp4

</>
) }
<EditTemplateNotification contentRef={ contentRef } />
<BackToPageNotification />
</>
);
}
2 changes: 0 additions & 2 deletions packages/edit-site/src/components/page-content-focus/index.js

This file was deleted.

Loading