Skip to content

Allow template part editing in write mode #67372

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

Merged
merged 16 commits into from
Dec 24, 2024
Merged

Allow template part editing in write mode #67372

Changes from 1 commit
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3616308
Allow template part editing in write mode
talldan Nov 28, 2024
e8c33f1
Update unit tests for template parts
talldan Nov 28, 2024
05542c6
Refine block editing mode calculation - allow editing of inner templa…
talldan Nov 27, 2024
0ebb6ea
Update tests for content blocks in template parts
talldan Nov 28, 2024
700f059
Add content role to site logo block
talldan Nov 28, 2024
508bd39
Consider template parts a section in navigation mode
talldan Dec 4, 2024
5e68eea
Allow content blocks to be explicitly disabled even when they have a …
talldan Dec 6, 2024
f609354
Avoid showing template parts when edting a page in Write Mode
talldan Dec 10, 2024
d6243e4
entries().some is not widely available, so switch to Array.from().some
talldan Dec 10, 2024
adde4d2
Update test to show explicitly disabled block
talldan Dec 10, 2024
0d4cf1d
Update unit tests
talldan Dec 11, 2024
673f4b0
Remove unused argument
talldan Dec 11, 2024
80d9048
Add optimization for disabled blocks calculation
talldan Dec 11, 2024
98f98cb
Avoid state updates when setting block editor modes to the same thing…
talldan Dec 11, 2024
e008553
Update DisableNonPageContentBlocks to not set block editor modes on a…
talldan Dec 11, 2024
0051caf
Try breaking DisableNonPageContentBlocks into separate useEffect calls
talldan Dec 11, 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
Prev Previous commit
Next Next commit
Avoid showing template parts when edting a page in Write Mode
  • Loading branch information
@talldan
talldan committed Dec 24, 2024
commit f609354411b86eb5f6373dda88b51bb95548f2d4
30 changes: 22 additions & 8 deletions packages/editor/src/components/provider/disable-non-page-content-blocks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@ import usePostContentBlocks from './use-post-content-blocks';
*/
export default function DisableNonPageContentBlocks() {
const contentOnlyIds = usePostContentBlocks();
const templateParts = useSelect( ( select ) => {
const { getBlocksByName } = select( blockEditorStore );
return getBlocksByName( 'core/template-part' );
const { templateParts, isNavigationMode } = useSelect( ( select ) => {
const { getBlocksByName, isNavigationMode: _isNavigationMode } =
select( blockEditorStore );
return {
templateParts: getBlocksByName( 'core/template-part' ),
isNavigationMode: _isNavigationMode(),
};
}, [] );
const disabledIds = useSelect(
( select ) => {
Expand All @@ -41,8 +45,10 @@ export default function DisableNonPageContentBlocks() {
for ( const clientId of contentOnlyIds ) {
setBlockEditingMode( clientId, 'contentOnly' );
}
for ( const clientId of templateParts ) {
setBlockEditingMode( clientId, 'contentOnly' );
if ( ! isNavigationMode ) {
for ( const clientId of templateParts ) {
setBlockEditingMode( clientId, 'contentOnly' );
}
Copy link
Contributor Author

@talldan talldan Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

When editing a page (with 'Show template' on) in write mode, template parts are now 'disabled' (because they're not set to 'contentOnly').

Copy link
Member

@ramonjd ramonjd Dec 18, 2024

Choose a reason for hiding this comment

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

Oh, I was just wondering about this, and what navigationMode even means in this context.

So in this third useEffect, it sets template part blocks to content-only mode if not in navigation mode (which seems to be another term for write mode)?

}
for ( const clientId of disabledIds ) {
setBlockEditingMode( clientId, 'disabled' );
Expand All @@ -55,15 +61,23 @@ export default function DisableNonPageContentBlocks() {
for ( const clientId of contentOnlyIds ) {
unsetBlockEditingMode( clientId );
}
for ( const clientId of templateParts ) {
unsetBlockEditingMode( clientId );
if ( ! isNavigationMode ) {
for ( const clientId of templateParts ) {
unsetBlockEditingMode( clientId );
}
}
for ( const clientId of disabledIds ) {
unsetBlockEditingMode( clientId );
}
} );
};
}, [ templateParts, contentOnlyIds, disabledIds, registry ] );
}, [
templateParts,
contentOnlyIds,
disabledIds,
isNavigationMode,
registry,
] );

return null;
}
Copy link
Contributor

@youknowriad youknowriad Dec 12, 2024

Choose a reason for hiding this comment

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

What would it take to move away from this component entirely? The code here is so confusing.

What happens if we passed a list of block types as settings to the block editor: editableBlockTypes: [ 'core/post-content', 'core/post-title' ] or something like that, would that be a valid solution?

Copy link
Contributor Author

@talldan talldan Dec 13, 2024

Choose a reason for hiding this comment

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

Yeah, it is confusing! There's already a list like this - https://github.com/WordPress/gutenberg/blob/trunk/packages/editor/src/components/provider/use-post-content-blocks.js

I think your idea works for most of the blocks, apart from the template part where the block itself is enabled but inner blocks are disabled. This is buggy anyway, as disabling the inner blocks like that doesn't prevent insertions, so at the moment you can still drag things into the template part block when editing a page. 😓

I personally think it'd be best to remove the way template parts are selectable in this mode. There's a parallel discussion on it here - #67621 (comment).

Copy link
Contributor Author

@talldan talldan Dec 13, 2024

Choose a reason for hiding this comment

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

Another thing is that in the long run, I think it'd be interesting to determine these blocks via their bindings.

E.g. if the user is editing a page with the id 12, then any blocks with bindings to that entity become editable.

That's a bit far away, but it makes me thing that rather than editableBlockTypes we should instead pass editableClientIds as the editor setting.

Blocks with a post meta binding could already be made to work like this if they're in the template.

Copy link
Contributor

@youknowriad youknowriad Dec 13, 2024

Choose a reason for hiding this comment

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

I think all these exceptions we're introducing to please some specific UX affordance are really not helping. We keep constructing and deconstructing. I'm not sure how to solve it personally but I think we should consider scaling back UX wise maybe and trying to systemize these changes.

It's really not clear to me why we're treating template parts differently than reusable blocks. We should (at least in our minds) consider these two things the same.

Copy link
Contributor Author

@talldan talldan Dec 17, 2024

Choose a reason for hiding this comment

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

Ok, I'll look into the template part / synced pattern disparity in a separate PR so design folk can be brought into a more focused conversation. My feeling is it might also take some improvements to both blocks to land in a happy place, but we'll see.

I'm happy to discuss possibilities around DisableNonPageContentBlocks, but that doesn't really seem like a blocker to this PR, as it's barely any different to trunk. Maybe there's someone else who'd also be interested in looking into this, I seem to be getting ten new things to do with each single PR I work on 😄

Copy link
Contributor Author

@talldan talldan Dec 17, 2024

Choose a reason for hiding this comment

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

Ok, I'll look into the template part / synced pattern disparity in a separate PR

#68042 is the PR.

Actually it does highlight something. Giving Site Title/Logo the role: content attribute isn't enough. In a pattern a paragraph would have this role, but it still isn't editable unless it is marked as a pattern override.

We likely need some additional mechanic to make this work.