Remove template parts from post content inserter an __unstable filter#37157
Merged
noisysocks merged 13 commits intotrunkfrom Dec 9, 2021
Merged
Conversation
adamziel
changed the title
|
Size Change: +249 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
adamziel
added
[Block] Template Part
youknowriad
reviewed
Dec 7, 2021
youknowriad
reviewed
Dec 7, 2021
Contributor
youknowriad
left a comment
youknowriad
left a comment
There was a problem hiding this comment.
I really like this (even if I don't like filters) because it allow us to express the requirements without assumption about what would be the best solution in the future (API)
ntsekouras
reviewed
Dec 7, 2021
| } else if ( hasParentAllowedBlock !== null && ! hasParentAllowedBlock ) { | ||
| return false; | ||
| } else if ( hasBlockAllowedParent !== null && ! hasBlockAllowedParent ) { | ||
| return false; |
Contributor
There was a problem hiding this comment.
Why we are changing the logic here if we are planning on using a filter? We want to apply the filter only in case of allowed blocks at this stage?
Contributor
Author
There was a problem hiding this comment.
That was my thinking, the filter allows you to change true return value to false. If it was to run on false, it would have to be there on every return` path, and there's plenty of them.
Contributor
Author
There was a problem hiding this comment.
I just simplified this logic further, should be easier to read now.
Contributor
|
The problem with the filter's approach is that they're not reactive: For example upon switching between "template mode" and non "template mode", the filter and selector won't necessarily be executed again. I think in our case now, it doesn't matter much because you can't switch between the two modes while the inserter stays open if I'm not wrong. But conceptually, having a filter inside a selector is bad pattern (and we should comment about it to avoid folks copying the pattern elsewhere) |
ntsekouras
reviewed
Dec 7, 2021
| } | ||
|
|
||
| const hasDisallowedParent = | ||
| getBlock( rootClientId )?.name === 'core/post-template' || |
Contributor
There was a problem hiding this comment.
I guess this should be core/post-content here, no?
Contributor
Author
There was a problem hiding this comment.
I started that way, but then I thought does it make sense to add template parts to post template?
So let me ask you now – does it? :-)
Contributor
There was a problem hiding this comment.
I guess it doesn't, but we should primarily check for post-content
Contributor
Author
There was a problem hiding this comment.
Fair point, I just added that.
Contributor
Author
|
@youknowriad all good points. I added a comment explaining our joint concerns. |
youknowriad
reviewed
Dec 7, 2021
| ) { | ||
| // Prevent adding template part in the post editor. | ||
| // Only add the filter when the post editor is initialized, not imported. | ||
| addFilter( |
Contributor
There was a problem hiding this comment.
I wonder if this should be inside a "useEffect" call as this is probably going to add a new filter on each render.
Contributor
Author
There was a problem hiding this comment.
That's what I initially did, only to find we're not inside of a React component here :D There's another listener registered in this function, a few dozen lines below. I believe this function is only called once:
gutenberg/lib/editor-settings.php
Lines 87 to 98 in 93bd8e9
<script id='wp-edit-post-js-after'>
( function() {
window._wpLoadBlockEditor = new Promise( function( resolve ) {
wp.domReady( function() {
resolve( wp.editPost.initializeEditor( 'editor', // ...
} );
} )();
noisysocks
approved these changes
Dec 9, 2021
Member
There was a problem hiding this comment.
I followed your testing steps and it worked like a charm. Adding an __unstable filter looks good as a temporary solution. Let's do it 👍
This is actually a perfect use case for an API that I proposed a long long time ago in #5540 (comment) and experimented with in #6346. We never ended up implementing what I proposed because it was overkill for what was needed at the time, but perhaps things are different now.
registerBlockType( 'core/template-part', {
parent: {
'post': false,
'core/post-template': false,
'core/post-content': false,
'*': true,
},
} );| 'blockEditor.__unstableCanInsertBlockType', | ||
| 'removeTemplatePartsFromPostTemplates', | ||
| ( | ||
| can, |
Member
There was a problem hiding this comment.
I'd name this canInsert for clarity.
Contributor
|
@noisysocks I think the difference is at the time |
Contributor
Author
|
@noisysocks this would work to an extent. If there's a hierarchy like:
Then the template part won't be available in that other editor. I'm not sure if that will ever be an actual use case, though. |
adamziel
force-pushed
the
try/filter-out-template-parts-using-workaround-callback
branch
from
66913d4 to
057dea9
Compare
Contributor
Author
|
I disabled the two e2e tests that specifically added template parts inside post editor. Let's see if all the checks turn green this time. |
Member
Ah right of course 🙂 |
noisysocks
deleted the
try/filter-out-template-parts-using-workaround-callback
branch
Member
That did indeed result in a ✅. Should we delete these tests? Or change them to use the site editor? |
This was referenced Dec 13, 2021
Contributor
Author
|
Both 😄 The skipped tests in |
Contributor
|
Shouldn't this be back ported for 5.9? -cc @noisysocks @mtias @tellthemachines |
youknowriad
added
the
Backport to WP 6.9 Beta/RC
noisysocks
removed
the
Backport to WP 6.9 Beta/RC
noisysocks
pushed a commit
that referenced
this pull request
Jan 4, 2022
…#37157) * Try a blockEditor.getInserterItems filter to remove template part from inserter items inside post templats * Filter based on name instead of id * Remove formatting changes * Add comment * add __unstable and name the filter * Add filter to edit-post package * Switch to a __unstableCanInsertBlockType filter * Lint and add comments * Restrict the filter to non-template posts * Explain why the filter approach will be removed in the future * Simplify boolean logic * Check for both core/post-template and core/post-content * Disable broken e2e tests
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Solves #30668
Following up on the discussion in #37109, this PR proposes a workaround to remove template parts from the post content editor without adding new stable APIs. The time is short and we are also past the feature freeze, so let's leave new, contexttual APIs for 6.0
The idea behind this PR is to filter the return value of
canInsertBlockTypeusing a globally-registered hook.The nice thing is that it prevents adding template parts using other means such as copying, pasting, and manually editing the code.
Test plan
template-partblock insidepost-templateorpost-content, but that you can add it in other places.template-partblock at all.template-partblock, except insidepost-templateorpost-content.cc @youknowriad @ntsekouras @gziolo