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

First pass at adding simple editing mode #65027

Closed
wants to merge 9 commits into from
Closed

First pass at adding simple editing mode #65027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6574a25
First pass at adding simple editing mode
noisysocks Sep 4, 2024
e1c5b75
Find the correct sectionRoot
getdave Sep 4, 2024
35a9b70
Use new selector to get section root
getdave Sep 4, 2024
ed283a5
Add selector to retrieve sections
getdave Sep 4, 2024
7be5cd0
Use new selector to get all sections
getdave Sep 4, 2024
4f29dcd
Show “Content” panel for sections when in “Simple” mode
getdave Sep 4, 2024
613c95e
Ensure section root is also contentOnly
getdave Sep 4, 2024
df42492
Reword terminology
getdave Sep 6, 2024
9dcebcf
Use DropdownMenuV2
richtabor Sep 9, 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
4 changes: 4 additions & 0 deletions lib/experimental/editor-settings.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ function gutenberg_enable_experiments() {
if ( $gutenberg_experiments && array_key_exists( 'gutenberg-zoom-out-experiment', $gutenberg_experiments ) ) {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalEnableZoomOutExperiment = true', 'before' );
}

if ( $gutenberg_experiments && array_key_exists( 'gutenberg-simple-editing-mode', $gutenberg_experiments ) ) {
wp_add_inline_script( 'wp-block-editor', 'window.__experimentalSimpleEditingMode = true', 'before' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't fell like we need an experiment if we take over the "select mode" and improve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we take over "Select Mode" do we need blockEditingMode at all, or can we just conditionalise all the block tools (in both locations) so that they

  • have the right visual appearance.
  • show only the necessary controls (i.e. respect the role=content attribute setting in block json)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think blockEditingMode probably needs to stay for BC reasons but I think ideally the block adapts to the tool without needing to call setBlockEditingMode explicitly.

}
}

add_action( 'admin_init', 'gutenberg_enable_experiments' );
Expand Down
13 changes: 13 additions & 0 deletions lib/experiments-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,19 @@ function gutenberg_initialize_experiments_settings() {
'id' => 'gutenberg-zoom-out-experiment',
)
);

add_settings_field(
'gutenberg-simple-editing-mode',
__( 'Simple editing mode', 'gutenberg' ),
'gutenberg_display_experiment_field',
'gutenberg-experiments',
'gutenberg_experiments_section',
array(
'label' => __( 'Add UI that lets you toggle between a simple and an advanced editing mode.', 'gutenberg' ),
'id' => 'gutenberg-simple-editing-mode',
)
);

register_setting(
'gutenberg-experiments',
'gutenberg-experiments'
Expand Down
14 changes: 13 additions & 1 deletion packages/block-editor/src/components/block-inspector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,13 +100,24 @@ const BlockInspector = ( { showNoBlockSelectedMessage = true } ) => {
getBlockName,
getContentLockingParent,
getTemplateLock,
getClosestSectionBlock,
getBlockEditingMode,
} = unlock( select( blockEditorStore ) );
const _selectedBlockClientId = getSelectedBlockClientId();
const _selectedBlockName =
_selectedBlockClientId && getBlockName( _selectedBlockClientId );
const _blockType =
_selectedBlockName && getBlockType( _selectedBlockName );

const closestSectionBlock = getClosestSectionBlock(
_selectedBlockClientId
);

const closestContentOnlySectionBlock =
getBlockEditingMode( closestSectionBlock ) === 'contentOnly'
? closestSectionBlock
: undefined;

return {
count: getSelectedBlockCount(),
selectedBlockClientId: _selectedBlockClientId,
Expand All @@ -117,7 +128,8 @@ const BlockInspector = ( { showNoBlockSelectedMessage = true } ) => {
( getTemplateLock( _selectedBlockClientId ) === 'contentOnly' ||
_selectedBlockName === 'core/block'
? _selectedBlockClientId
: undefined ),
: undefined ) ||
closestContentOnlySectionBlock,
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of the changes in this file is that we want to show the "inspector control" of the parent "content-only" block when it exists rather than the leaf block that is being selected.

Are we doing the same for the block toolbar. This also feels like something we want to do consistently for block inspector and block toolbar?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change can probably be extracted to a dedicated PR as it can be seen as an improvement to content only blocks. would be good to e2e test as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

That change is incorrect. I've been looking at how to amend it so that:

  • you see the Content panel [of the nearest "Section"] in the inspector controls. Similar to when setting templateLock: contentOnly on a Group block.
  • you see the Inspector Controls (contentOnly version) for the selected block

Copy link
Contributor

Choose a reason for hiding this comment

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

you see the Content panel [of the nearest "Section"] in the inspector controls. Similar to when setting templateLock: contentOnly on a Group block.
you see the Inspector Controls (contentOnly version) for the selected block

If we want to combine two blocks in the inspector controls, I feel we need some design explorations no? Or do we just dump everything there? It maybe hard for users to understand what's happening no?

Copy link
Contributor

Choose a reason for hiding this comment

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

@richtabor What are your thoughts here? If you select a child block of a section, should we be exposing the currently selected block's inspector controls or just the parent "section" block's controls?

If it's the former how can we combine that with the Content panel being shown at all times without causing overwhelm?

Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding of the changes in this file is that we want to show the "inspector control" of the parent "content-only" block when it exists rather than the leaf block that is being selected.
Are we doing the same for the block toolbar. This also feels like something we want to do consistently for block inspector and block toolbar?

I think this is already how contentOnly template locking works, as does the pattern block.

It's something that's always felt a little off to me (having a different selected block shown at the top of the inspector compared to the block toolbar), but I think it might be worth breaking it out into a separate issue to discuss given the precedence.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should be consistent with what happens when you have a contentOnly pattern for now.

Wouldn't mind changing it in a separate PR though. Either to use a drill-down UI like we do for the Navigation block or to just get rid of it entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

Drill down might work

};
}, [] );

Expand Down
40 changes: 40 additions & 0 deletions packages/block-editor/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,46 @@ export function isZoomOutMode( state ) {
return state.editorMode === 'zoom-out';
}

/**
* Retrieves the client ID for the block that is acting as the element
* which contains the "sections" of a template/post.
*
* @param {Object} state - The current state.
* @return {string|undefined} The root client ID for the section, or undefined if not set.
*/
export function getSectionRootClientId( state ) {
return state.settings?.[ sectionRootClientIdKey ];
}

/**
* Retrieves the client IDs for the individual "sections"
* within the current template/post.
*
* @param {Object} state - The current state.
* @return {Array} An array of client IDs for the blocks within the section.
*/
export function getSectionClientIds( state ) {
const sectionRootClientId = getSectionRootClientId( state );
return getBlockOrder( state, sectionRootClientId );
}

/**
* Retrieves the closest "section" block to the given client ID.
*
* @param {Object} state - The current state.
* @param {string} clientId - The client ID to start the search from.
* @return {string|undefined} The client ID of the closest section block, or undefined if not found.
*/
export function getClosestSectionBlock( state, clientId ) {
let current = clientId;
let result;
const sectionClientIds = getSectionClientIds( state );
while ( current ) {
if ( sectionClientIds.includes( current ) ) {
result = current;
break;
}
current = state.blocks.parents.get( current );
}
return result;
}
7 changes: 6 additions & 1 deletion packages/editor/src/components/document-tools/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import { unlock } from '../../lock-unlock';
import { store as editorStore } from '../../store';
import EditorHistoryRedo from '../editor-history/redo';
import EditorHistoryUndo from '../editor-history/undo';
import SimpleEditingModeSelector from '../simple-editing-mode-selector';

function DocumentTools( { className, disableBlockTools = false } ) {
const { setIsInserterOpened, setIsListViewOpened } =
Expand Down Expand Up @@ -139,7 +140,11 @@ function DocumentTools( { className, disableBlockTools = false } ) {
<>
{ isLargeViewport && ! hasFixedToolbar && (
<ToolbarItem
as={ ToolSelector }
as={
window.__experimentalSimpleEditingMode
? SimpleEditingModeSelector
: ToolSelector
Copy link
Contributor

@youknowriad youknowriad Sep 10, 2024 •
edited by getdave
Loading

Choose a reason for hiding this comment

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

I'm not sure why we're using a separate tool selector personally, can't we reuse the same?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was just for the purposes of getting an experimnet up without editing the other component. We should be able to reuse the ToolSelector - especially if we use Select mode.

Copy link
Member Author

Choose a reason for hiding this comment

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

It had to be a new component so that it could live in packages/editor where rendering mode is (not packages/block-editor where editing mode is) and so that we could switch it when the experiment is enabled/disabled. If we're okay with using editor mode (see other thread) then yeah we can re-use the existing component.

}
showTooltip={ ! showIconLabels }
variant={
showIconLabels ? 'tertiary' : undefined
Expand Down
100 changes: 100 additions & 0 deletions packages/editor/src/components/provider/content-only-lock-sections.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
/**
* WordPress dependencies
*/
import { useSelect, useDispatch, useRegistry } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';
import { useEffect } from '@wordpress/element';
import { store as blocksStore } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';

/**
* Component that when rendered, makes it so that the editor allows only
* specific sections to be edited in content-only mode for templates.
*/
export default function ContentOnlyLockSections() {
const registry = useRegistry();

const { setBlockEditingMode, unsetBlockEditingMode } =
useDispatch( blockEditorStore );

const selected = useSelect( ( select ) => {
const {
getClientIdsOfDescendants,
getClientIdsWithDescendants,
getSectionRootClientId,
getSectionClientIds,
} = unlock( select( blockEditorStore ) );

const sectionRootClientId = getSectionRootClientId();
const sectionClientIds = getSectionClientIds();
const allClientIds = sectionRootClientId
? getClientIdsOfDescendants( sectionRootClientId )
: getClientIdsWithDescendants();
Comment on lines +34 to +36
Copy link
Contributor

Choose a reason for hiding this comment

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

This code is appearing in various PRs. I think we should have a selector which finds the "sections". I don't think it's a premature abstraction because it will make the code easier to reason about.

return {
sectionRootClientId,
sectionClientIds,
allClientIds,
};
}, [] );
const { sectionClientIds, allClientIds, sectionRootClientId } = selected;
const { getBlockOrder, getBlockName } = useSelect( blockEditorStore );
const { __experimentalHasContentRoleAttribute } = useSelect( blocksStore );

useEffect( () => {
const sectionChildrenClientIds = sectionClientIds.flatMap(
( clientId ) => getBlockOrder( clientId )
);
const contentClientIds = allClientIds.filter( ( clientId ) =>
__experimentalHasContentRoleAttribute( getBlockName( clientId ) )
);

registry.batch( () => {
// 1. The section root should hide non-content controls.
setBlockEditingMode( sectionRootClientId, 'contentOnly' );

// 2. Each section should hide non-content controls.
for ( const clientId of sectionClientIds ) {
setBlockEditingMode( clientId, 'contentOnly' );
}

// 3. The children of the section should be disabled.
for ( const clientId of sectionChildrenClientIds ) {
setBlockEditingMode( clientId, 'disabled' );
}

// 4. ...except for the "content" blocks which should be content-only.
for ( const clientId of contentClientIds ) {
setBlockEditingMode( clientId, 'contentOnly' );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we start moving away from setBlockEditingMode usage and instead update the "getBlockEditingMode" selector to guess the right mode based on the state instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. So rather than batching multiple actions, instead we make the selector more complex so that it can infer the correct block editing mode on a per block basis?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, that makes the code more solid IMO. because it's easy for folks to render competing components like the ContentOnlyLockSections here and you end up with race conditions. For instance, I know we had the navigation block at some point call setBlockEditingMode directly which conflicted with the locking applied based on the "editor package rendering mode" and the more we add this kind of components, the worse it gets.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take a look at this. I'd also love to solve the similar situation in the pattern block, though I wonder if it'd be ok to build so much into one selector.

Copy link
Contributor

Choose a reason for hiding this comment

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

@talldan worth noting that I already implemented this for the "select mode" in this PR #65204

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was that folks would use useBlockEditingMode in their blocks but yeah unfortunately it's become more common to call setBlockEditingMode directly which has nasty side effects. Need to revisit this API in favour of something more declarative. Maybe an array of "locks" that is passed in via block settings.

} );

return () => {
registry.batch( () => {
for ( const clientId of [
sectionRootClientId,
...sectionClientIds,
...sectionChildrenClientIds,
...contentClientIds,
] ) {
unsetBlockEditingMode( clientId );
}
} );
};
}, [
__experimentalHasContentRoleAttribute,
allClientIds,
getBlockName,
getBlockOrder,
registry,
sectionRootClientId,
sectionClientIds,
setBlockEditingMode,
unsetBlockEditingMode,
] );

return null;
}
13 changes: 10 additions & 3 deletions packages/editor/src/components/provider/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ import EditorKeyboardShortcuts from '../global-keyboard-shortcuts';
import PatternRenameModal from '../pattern-rename-modal';
import PatternDuplicateModal from '../pattern-duplicate-modal';
import TemplatePartMenuItems from '../template-part-menu-items';
import { TEMPLATE_POST_TYPE } from '../../store/constants';
import ContentOnlyLockSections from './content-only-lock-sections';

const { ExperimentalBlockEditorProvider } = unlock( blockEditorPrivateApis );
const { PatternsMenuItems } = unlock( editPatternsPrivateApis );
Expand Down Expand Up @@ -302,9 +304,14 @@ export const ExperimentalEditorProvider = withRegistryProvider(
<PatternsMenuItems />
<TemplatePartMenuItems />
<ContentOnlySettingsMenu />
{ mode === 'template-locked' && (
<DisableNonPageContentBlocks />
) }
{ mode === 'template-locked' &&
type === TEMPLATE_POST_TYPE && (
<ContentOnlyLockSections />
) }
{ mode === 'template-locked' &&
type !== TEMPLATE_POST_TYPE && (
<DisableNonPageContentBlocks />
) }
{ type === 'wp_navigation' && (
<NavigationBlockEditingMode />
) }
Expand Down
6 changes: 5 additions & 1 deletion packages/editor/src/components/provider/use-block-editor-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import { mediaUpload } from '../../utils';
import { store as editorStore } from '../../store';
import { unlock } from '../../lock-unlock';
import { useGlobalStylesContext } from '../global-styles-provider';
import { TEMPLATE_POST_TYPE } from '../../store/constants';

const EMPTY_BLOCKS_LIST = [];
const EMPTY_OBJECT = {};
Expand Down Expand Up @@ -142,7 +143,10 @@ function useBlockEditorSettings( settings, postType, postId, renderingMode ) {
: undefined;

function getSectionRootBlock() {
if ( renderingMode === 'template-locked' ) {
if (
renderingMode === 'template-locked' &&
postType !== TEMPLATE_POST_TYPE
) {
return getBlocksByName( 'core/post-content' )?.[ 0 ] ?? '';
}

Expand Down
97 changes: 97 additions & 0 deletions packages/editor/src/components/simple-editing-mode-selector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import {
Button,
SVG,
Path,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { useSelect, useDispatch } from '@wordpress/data';
import { forwardRef } from '@wordpress/element';
import { edit } from '@wordpress/icons';

/**
* Internal dependencies
*/
import { unlock } from '../../lock-unlock';
import { store as editorStore } from '../../store';
import { TEMPLATE_POST_TYPE } from '../../store/constants';

const { DropdownMenuV2 } = unlock( componentsPrivateApis );

const selectIcon = (
<SVG
xmlns="http://www.w3.org/2000/svg"
width="24"
height="24"
viewBox="0 0 24 24"
>
<Path d="M9.4 20.5L5.2 3.8l14.6 9-2 .3c-.2 0-.4.1-.7.1-.9.2-1.6.3-2.2.5-.8.3-1.4.5-1.8.8-.4.3-.8.8-1.3 1.5-.4.5-.8 1.2-1.2 2l-.3.6-.9 1.9zM7.6 7.1l2.4 9.3c.2-.4.5-.8.7-1.1.6-.8 1.1-1.4 1.6-1.8.5-.4 1.3-.8 2.2-1.1l1.2-.3-8.1-5z" />
</SVG>
);

function SimpleEditingModeSelector( props, ref ) {
const { postType, renderingMode } = useSelect(
( select ) => ( {
postType: select( editorStore ).getCurrentPostType(),
renderingMode: select( editorStore ).getRenderingMode(),
} ),
[]
);

const { setRenderingMode } = useDispatch( editorStore );
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand why we're using the "rendering mode" instead of the editor mode (navigation mode) for this. These two feel to me like different things and that we shouldn't be touching the rendering modes for this PR.

Copy link
Contributor

@talldan talldan Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I expect it was a quick way to get up and running. There a couple of things that need to be solved with using editor mode:

  • Editor mode already uses a value of edit to represent what we want to call 'design'. Not unsolvable, but definitely awkward. We might want to deprecate the current values and go for something like design and content as the values.
  • From a user perspective, the editing mode seems like it can work alongside zoom-out mode, but zoom-out is already an editor mode so with the way it works right now the two couldn't be active at the same time. Seems like we'd need to figure out how we want the two to work together and exactly what the right primitives are for representing these modes.

Copy link
Member Author

@noisysocks noisysocks Sep 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I used rendering mode because it's in editorStore not blockEditorStore and I was thinking that this feature isn't really something that all block editors should have. (Maybe it is, though.)

Copy link
Member Author

Choose a reason for hiding this comment

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

From a user perspective, the editing mode seems like it can work alongside zoom-out mode

I don't think we should allow this. When zoomed out, the text is small and not conducive to editing.

To me there's three "modes":

  • Zoomed out. Focus is on patterns and previewing the whole page
  • Edit mode. Focus is on patterns and editing the content
  • Design mode. Focus is on blocks

So yeah sounds like editor mode is a better fit 👍


if ( postType !== TEMPLATE_POST_TYPE ) {
return null;
}

return (
<DropdownMenuV2
align="start"
trigger={
<Button
{ ...props }
ref={ ref }
icon={
renderingMode === 'template-locked' ? selectIcon : edit
}
label={ __( 'Editing mode' ) }
size="small"
/>
}
>
<DropdownMenuV2.Group>
<DropdownMenuV2.RadioItem
name="editing-mode"
value="simple"
checked={ renderingMode === 'template-locked' }
onChange={ () => setRenderingMode( 'template-locked' ) }
>
<DropdownMenuV2.ItemLabel>
{ __( 'Edit' ) }
</DropdownMenuV2.ItemLabel>
<DropdownMenuV2.ItemHelpText>
{ __( 'Focus on content.' ) }
</DropdownMenuV2.ItemHelpText>
</DropdownMenuV2.RadioItem>
<DropdownMenuV2.RadioItem
name="editing-mode"
value="advanced"
checked={ renderingMode !== 'template-locked' }
onChange={ () => setRenderingMode( 'post-only' ) }
>
<DropdownMenuV2.ItemLabel>
{ __( 'Design' ) }
</DropdownMenuV2.ItemLabel>
<DropdownMenuV2.ItemHelpText>
{ __( 'Full control over layout and styling.' ) }
</DropdownMenuV2.ItemHelpText>
</DropdownMenuV2.RadioItem>
</DropdownMenuV2.Group>
</DropdownMenuV2>
);
}

export default forwardRef( SimpleEditingModeSelector );
9 changes: 9 additions & 0 deletions packages/editor/src/components/simple-editing-mode-selector/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
.editor-simple-editing-mode-selector__menu {
.components-menu-item__button {
height: auto;
}

.components-menu-item__info {
width: max-content;
}
}
1 change: 1 addition & 0 deletions packages/editor/src/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
@import "./components/start-page-options/style.scss";
@import "./components/start-template-options/style.scss";
@import "./components/sidebar/style.scss";
@import "./components/simple-editing-mode-selector/style.scss";
@import "./components/site-discussion/style.scss";
@import "./components/table-of-contents/style.scss";
@import "./components/text-editor/style.scss";
Expand Down
Loading