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

Post Content Block: Disable when editing content #24006

Closed
wants to merge 6 commits into from
Closed

Post Content Block: Disable when editing content #24006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
17b11b0
Post Content Block: Disable when editing content
ockham Jul 16, 2020
883524b
Don't register Post Content block on the client side if editing content
ockham Jul 21, 2020
3581cf0
Revert "Post Content Block: Disable when editing content"
ockham Jul 21, 2020
c2532e1
Make extra arg optional
ockham Jul 21, 2020
d5e6b14
Rephrase comment
ockham Jul 21, 2020
43e4a90
Load FSE blocks with wp_template context in test
ockham Jul 23, 2020
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
19 changes: 15 additions & 4 deletions packages/block-library/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,9 @@ export const registerCoreBlocks = () => {
/**
* Function to register experimental core blocks depending on editor settings.
*
* @param {Object} settings Editor settings.
* @param {Object} settings Editor settings.
* @param {Object} [context] Post context.
* @param {string} [context.postType] Post Type
*
* @example
* ```js
Expand All @@ -185,7 +187,7 @@ export const registerCoreBlocks = () => {
*/
export const __experimentalRegisterExperimentalCoreBlocks =
process.env.GUTENBERG_PHASE === 2
? ( settings ) => {
? ( settings, { postType } = {} ) => {
const {
__experimentalEnableLegacyWidgetBlock,
__experimentalEnableFullSiteEditing,
Expand All @@ -208,7 +210,16 @@ export const __experimentalRegisterExperimentalCoreBlocks =
queryLoop,
queryPagination,
postTitle,
postContent,
/*
* Only provide the Post Content block when editing templates or,
* template parts so there's no risk of posts including themselves
* through a Post Content block (leading to infinite recursion).
*/
[ 'wp_template', 'wp_template_part' ].includes(
postType
)
? postContent
Copy link
Member

Choose a reason for hiding this comment

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

should we exclude more site blocks, like postAuthor or postComments, etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤔 The main rationale for this PR was to prevent blockception (infinite recursion), which wouldn't happen with those blocks. But yeah, we might want to exclude the class of FSE relevant post... blocks from the content editor. They used to be kind of handy for testing, but maybe that's no longer needed now that we can do that in the Site Editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please no, or at least not yet! 😄
The site editor is unreliable when it comes to loading post attributes into FSE blocks, while it's very easy to test post blocks in the post editor.
I agree that we should eventually limit FSE blocks to the Site Editor, but let's do it once we make it work appropriately.

: null,
postAuthor,
postComments,
postCommentsCount,
Expand All @@ -217,7 +228,7 @@ export const __experimentalRegisterExperimentalCoreBlocks =
postExcerpt,
postFeaturedImage,
postTags,
]
].filter( Boolean )
Addison-Stavlo marked this conversation as resolved.
Show resolved Hide resolved
: [] ),
].forEach( registerBlock );
}
Expand Down
2 changes: 1 addition & 1 deletion packages/edit-post/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ export function initializeEditor(
);
registerCoreBlocks();
if ( process.env.GUTENBERG_PHASE === 2 ) {
__experimentalRegisterExperimentalCoreBlocks( settings );
__experimentalRegisterExperimentalCoreBlocks( settings, { postType } );
noahtallen marked this conversation as resolved.
Show resolved Hide resolved
}

// Show a console log warning if the browser is not in Standards rendering mode.
Expand Down
4 changes: 3 additions & 1 deletion packages/edit-site/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,9 @@ export function initialize( id, settings ) {

registerCoreBlocks();
if ( process.env.GUTENBERG_PHASE === 2 ) {
__experimentalRegisterExperimentalCoreBlocks( settings );
__experimentalRegisterExperimentalCoreBlocks( settings, {
postType: 'wp_template',
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Jul 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this could be a little confusing. postType makes a lot of sense in edit-post, but thinking of this for edit-site it seems a bit more funky. Is the post type always wp_template in edit-site? Even when we are selecting specific pages or posts? If we are going to hardcode a value here maybe it should just be something like 'edit_site'.

Perhaps something like editorType, editorEntity, or some better name I cant think of atm instead of postType.
So for edit post we would pass in { editorType: postType }.
For site editor { editorType: 'site_editor' }, and add that as a 3rd valid option in the array checked in the ternary?

Or maybe just a comment here explaining why we are going with wp_template as the hardcoded value, and some block(s) wont be registered if this isn't passed as wp_template or wp_template_part. 🤷‍♀️

Copy link
Contributor Author

@ockham ockham Jul 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I feel like this could be a little confusing. postType makes a lot of sense in edit-post, but thinking of this for edit-site it seems a bit more funky. Is the post type always wp_template in edit-site? Even when we are selecting specific pages or posts? If we are going to hardcode a value here maybe it should just be something like 'edit_site'.

Yeah, it will be wp_template in those cases, too -- it's still the wp_template CPT we're editing. (However, it can also be wp_template_part, if we're editing a template part.)

A few factors come into play here. One is that we're only setting postType upon initialization -- we don't re-initialize upon switching between templates or template parts. We know that the Site Editor initially always shows a template (and not a template part) -- the home page template -- so we hard-code to wp_template. It just so happens that template parts allow for the same block types (and disallow Post Content).

Perhaps something like editorType, editorEntity, or some better name I cant think of atm instead of postType.
So for edit post we would pass in { editorType: postType }.
For site editor { editorType: 'site_editor' }, and add that as a 3rd valid option in the array checked in the ternary?

Yep, which brings us to the question whether postType is the right argument to base these allow/deny lists on anyway. The reason for choosing postType (but really post context -- the second argument to __experimentalRegisterExperimentalCoreBlocks is an object that would also allow for additional fields such as e.g. post ID etc) is that it aligns with the allowed_block_types filter, which accepts a post object. Arguably, our client-side filter should be aligned with the server-side one, API-wise, since they're semantically doing the same thing.

However, it's arguable that a post object simply isn't the completely right fit anyway, and that we should replace it with something like editorType. Coincidentally, @TimothyBJacobs arrived at a similar conclusion in a separate issue (about providing the list of allowed blocks via the REST API). So maybe it's time to consider that, and maybe even deprecate/replace the current allowed_block_types filter (with a filter that accepts a context rather than a post argument) (cc/ @youknowriad) We would lose the ability to filter on a per-post basis, but OTOH, a post object might also not be the best fit for some (edge?) cases that Timothy lists in that discussion.

Or maybe just a comment here explaining why we are going with wp_template as the hardcoded value, and some block(s) wont be registered if this isn't passed as wp_template or wp_template_part. 🤷‍♀️

Agree, writing this reply I realized there's a lot of considerations that have gone into this.

} );
}

render( <Editor />, document.getElementById( id ) );
Expand Down
10 changes: 9 additions & 1 deletion test/integration/full-content/full-content.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,15 @@ describe( 'full post content fixture', () => {
require( '../../../packages/editor/src/hooks' );
registerCoreBlocks();
if ( process.env.GUTENBERG_PHASE === 2 ) {
__experimentalRegisterExperimentalCoreBlocks( settings );
/**
* We're loading these blocks with a `wp_template` post context,
* since most of them are meant to be used in Site Editor templates,
* to the point that e.g. the Post Content block is unavailable in
* a Post Editor (`post`) context.
*/
__experimentalRegisterExperimentalCoreBlocks( settings, {
postType: 'wp_template',
} );
}
} );

Expand Down