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

Template Parts: allow selection in page content focus mode #60010

Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ function BlockSwitcherDropdownMenuContents( {
);
}

export const BlockSwitcher = ( { clientIds } ) => {
export const BlockSwitcher = ( { clientIds, disabled } ) => {
const {
canRemove,
hasBlockStyles,
Expand Down Expand Up @@ -229,9 +229,8 @@ export const BlockSwitcher = ( { clientIds } ) => {
const blockSwitcherLabel = isSingleBlock
? blockTitle
: __( 'Multiple blocks selected' );
const hideDropdown = ! hasBlockStyles && ! canRemove;

if ( hideDropdown ) {
if ( disabled || ( ! hasBlockStyles && ! canRemove ) ) {
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
return (
<ToolbarGroup>
<ToolbarButton
Expand Down
41 changes: 22 additions & 19 deletions packages/block-editor/src/components/block-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,26 +170,29 @@ export function PrivateBlockToolbar( {
{ isUsingBindings && canBindBlock( blockName ) && (
<BlockBindingsIndicator />
) }
{ ( shouldShowVisualToolbar || isMultiToolbar ) &&
isDefaultEditingMode && (
retrofox marked this conversation as resolved.
Show resolved Hide resolved
<div
ref={ nodeRef }
{ ...showHoveredOrFocusedGestures }
>
<ToolbarGroup className="block-editor-block-toolbar__block-controls">
<BlockSwitcher clientIds={ blockClientIds } />
{ ! isMultiToolbar && (
<BlockLockToolbar
clientId={ blockClientId }
{ ( shouldShowVisualToolbar || isMultiToolbar ) && (
<div ref={ nodeRef } { ...showHoveredOrFocusedGestures }>
<ToolbarGroup className="block-editor-block-toolbar__block-controls">
<BlockSwitcher
clientIds={ blockClientIds }
disabled={ ! isDefaultEditingMode }
/>
{ isDefaultEditingMode && (
<>
{ ! isMultiToolbar && (
<BlockLockToolbar
clientId={ blockClientId }
/>
) }
<BlockMover
clientIds={ blockClientIds }
hideDragHandle={ hideDragHandle }
/>
) }
<BlockMover
clientIds={ blockClientIds }
hideDragHandle={ hideDragHandle }
/>
</ToolbarGroup>
</div>
) }
</>
) }
</ToolbarGroup>
</div>
) }
<Shuffle clientId={ blockClientId } />
{ shouldShowVisualToolbar && isMultiToolbar && (
<BlockGroupToolbar />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
store as blockEditorStore,
privateApis as blockEditorPrivateApis,
} from '@wordpress/block-editor';
import { useMemo } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -15,15 +14,16 @@ import { unlock } from '../../../lock-unlock';

const { BlockQuickNavigation } = unlock( blockEditorPrivateApis );

const PAGE_CONTENT_BLOCKS = [
'core/post-content',
'core/post-featured-image',
'core/post-title',
];

export default function PageContent() {
const clientIdsTree = useSelect(
( select ) =>
unlock( select( blockEditorStore ) ).getEnabledClientIdsTree(),
[]
);
const clientIds = useMemo(
() => clientIdsTree.map( ( { clientId } ) => clientId ),
[ clientIdsTree ]
);
const clientIds = useSelect( ( select ) => {
const { getBlocksByName } = select( blockEditorStore );
return getBlocksByName( PAGE_CONTENT_BLOCKS );
}, [] );
return <BlockQuickNavigation clientIds={ clientIds } />;
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,49 +5,68 @@ import { useSelect, useDispatch } from '@wordpress/data';
import { store as blockEditorStore } from '@wordpress/block-editor';
import { useEffect } from '@wordpress/element';

const PAGE_CONTENT_BLOCKS = [
'core/post-title',
'core/post-featured-image',
const CONTENT_ONLY_BLOCKS = [
'core/post-content',
'core/post-featured-image',
'core/post-title',
'core/template-part',
];

function useDisableNonPageContentBlocks() {
const contentIds = useSelect( ( select ) => {
/**
* Component that when rendered, makes it so that the site editor allows only
* page content to be edited.
*/
export default function DisableNonPageContentBlocks() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not specific to this PR but I see this component as a hack. Also the setBlockEditingMode and useBlockEditingMode APIs have proven to be problematic in a lot of ways. Multiple conflicts and race conditions, I'd love to absorb these in the block editor framework/package instead (maybe through a setting or something) but we'd need to think properly about the API that we'd want to provide there.

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 agreed and I keep running into limitations too. For example in this PR I want to disable insertion but not selection on the Template Part and disable all of its children. contentOnly doesn't really cut it for the former. There were similar issues with synced patterns.

We also have lots of ways to do similar things. I feel that with the right abstraction we could unify tempateLock, the lock attribute, and setBlockEditingMode.

Copy link
Contributor

Choose a reason for hiding this comment

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

We also have lots of ways to do similar things. I feel that with the right abstraction we could unify tempateLock, the lock attribute, and setBlockEditingMode.

I prefer templateLock instead because it's declarative while setBlockEditingMode is more subject to race conditions. I think the framework should be able to "guess" the mode, rather the consumer providing it for each block.

const contentOnlyIds = useSelect( ( select ) => {
const { getBlocksByName, getBlockParents, getBlockName } =
select( blockEditorStore );
return getBlocksByName( PAGE_CONTENT_BLOCKS ).filter( ( clientId ) =>
return getBlocksByName( CONTENT_ONLY_BLOCKS ).filter( ( clientId ) =>
getBlockParents( clientId ).every( ( parentClientId ) => {
const parentBlockName = getBlockName( parentClientId );
return (
// Ignore descendents of the query block.
parentBlockName !== 'core/query' &&
! PAGE_CONTENT_BLOCKS.includes( parentBlockName )
// Enable only the top-most block.
! CONTENT_ONLY_BLOCKS.includes( parentBlockName )
);
} )
);
}, [] );

const disabledIds = useSelect( ( select ) => {
const { getBlocksByName, getBlockOrder } = select( blockEditorStore );
return getBlocksByName( [ 'core/template-part' ] ).flatMap(
( clientId ) => getBlockOrder( clientId )
);
}, [] );

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

useEffect( () => {
setBlockEditingMode( '', 'disabled' ); // Disable editing at the root level.

for ( const contentId of contentIds ) {
setBlockEditingMode( contentId, 'contentOnly' ); // Re-enable each content block.
setBlockEditingMode( '', 'disabled' );
for ( const clientId of contentOnlyIds ) {
setBlockEditingMode( clientId, 'contentOnly' );
}
for ( const clientId of disabledIds ) {
setBlockEditingMode( clientId, 'disabled' );
}

return () => {
unsetBlockEditingMode( '' );
for ( const contentId of contentIds ) {
unsetBlockEditingMode( contentId );
for ( const clientId of contentOnlyIds ) {
unsetBlockEditingMode( clientId );
}
for ( const clientId of disabledIds ) {
unsetBlockEditingMode( clientId );
}
};
}, [ contentIds, setBlockEditingMode, unsetBlockEditingMode ] );
}
}, [
contentOnlyIds,
disabledIds,
setBlockEditingMode,
unsetBlockEditingMode,
] );

/**
* Component that when rendered, makes it so that the site editor allows only
* page content to be edited.
*/
export default function DisableNonPageContentBlocks() {
useDisableNonPageContentBlocks();
return null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,13 @@ describe( 'DisableNonPageContentBlocks', () => {
getBlockName( state, clientId ) {
return testBlocks[ clientId ];
},
getBlockOrder( state, rootClientId ) {
return Object.keys( testBlocks ).filter(
( clientId ) =>
clientId.startsWith( rootClientId ) &&
clientId !== rootClientId
);
},
},
actions: {
setBlockEditingMode,
Expand All @@ -69,22 +76,36 @@ describe( 'DisableNonPageContentBlocks', () => {
</RegistryProvider>
);

expect( setBlockEditingMode.mock.calls ).toEqual( [
[ '', 'disabled' ], // root
[ '10', 'contentOnly' ], // post-title
[ '11', 'contentOnly' ], // post-featured-image
[ '12', 'contentOnly' ], // post-content
// NOT the post-featured-image nested within post-content
// NOT any of the content blocks within query
] );
expect( setBlockEditingMode.mock.calls ).toEqual(
expect.arrayContaining( [
[ '', 'disabled' ], // root
[ '0', 'contentOnly' ], // core/template-part
[ '00', 'disabled' ], // core/site-title
[ '01', 'disabled' ], // core/navigation
[ '10', 'contentOnly' ], // post-title
[ '11', 'contentOnly' ], // post-featured-image
[ '12', 'contentOnly' ], // post-content
[ '3', 'contentOnly' ], // core/template-part
[ '30', 'disabled' ], // core/paragraph
// NOT the post-featured-image nested within post-content
// NOT any of the content blocks within query
] )
);

unmount();

expect( unsetBlockEditingMode.mock.calls ).toEqual( [
[ '' ], // root
[ '10' ], // post-title
[ '11' ], // post-featured-image
[ '12' ], // post-content
] );
expect( unsetBlockEditingMode.mock.calls ).toEqual(
expect.arrayContaining( [
[ '' ], // root
[ '0' ], // core/template-part
[ '00' ], // core/site-title
[ '01' ], // core/navigation
[ '10' ], // post-title
[ '11' ], // post-featured-image
[ '12' ], // post-content
[ '3' ], // core/template-part
[ '30' ], // core/paragraph
] )
);
} );
} );
Loading