Mark applying block templates not persistent #45843
Merged
Conversation
This file contains 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
|
|
|
Size Change: +14.8 kB (+1%) Total Size: 1.31 MB
ℹ️ View Unchanged
|
|
Well the e2e tests here give some sense of confidence I think. Do you think there's a test we should add here? |
3 tasks
Could be good to check for regressions of the issue this fixes. Possibly a template that's initially empty and then populated after a Not sure how easy it is to construct a test like that, not sure what the existing template coverage is like. |
draganescu
added
[Type] Enhancement
|
It might be a good time to check existing template test coverage. For example, do we have tests that track if the template synchronization works correctly? P.S. I'll add e2e tests for #45632 to avoid similar regressions in the future. |
|
Ok so I managed to get to this repro code: Expand for codeconst el = wp.element.createElement;
const useState = wp.element.useState;
const InnerBlocks = wp.blockEditor.InnerBlocks;
const TEMPLATE_TWO_PARAGRAPHS = [
[
'core/paragraph',
{
fontSize: 'large',
content: 'One',
},
],
[
'core/paragraph',
{
fontSize: 'large',
content: 'Two',
},
],
];
window.wp.blocks.registerBlockType( 'test/test-inner-blocks-async-template', {
title: 'Test Inner Blocks no locking',
icon: 'cart',
category: 'text',
edit() {
const [ template, setTemplate ] = useState([]);
setInterval( () => {
setTemplate( TEMPLATE_TWO_PARAGRAPHS );
}, 1000 );
return el( InnerBlocks, {
template: template,
} );
},
save() {
return el( InnerBlocks.Content );
},
} );
const testBlock = window.wp.blocks.createBlock ( 'test/test-inner-blocks-async-template', {} )
window.wp.data.dispatch( 'core/block-editor' ).insertBlock( testBlock );Pasting this and running it in the console:
How do best implement this as a test? If the console test is good, and in the interest of speed (I'll be away for a bit) if anyone can contribute the above code as a test to this branch I'd be ❤️
Otherwise I'll pick it up back when I return. |
|
The regression test would be great, but also IMO, the main thing to manually test before merging this is that it doesn't break other situations where templates are used. It's pretty easy to assert that this solves the known situation, but it'd be good to get a handle on whether there are situations that we don't know about by looking at other usages of templates in the codebase. The main one is - could there be a situation in which the content should be marked as dirty, but this change breaks that. Looking at the code it's unlikely, but some reassurance would be good. |
Can't think of any either but it's hard to be certain know before actually shipping. |
|
@draganescu I tried adding a test, but it didn't work - my tests pass in trunk as well as in this branch. I added the code as a commit to show how far I've got in case you (or anyone else) is able to pick it up... |
- Move block code to test plugin file - Move from block locking test file to a new inner blocks template file - Update test logic to test post is not dirty on a fresh load - Rename post type templates file to distinguish from inner blocks templates file
talldan
reviewed
Nov 25, 2022
| test.describe( 'Inner blocks templates', () => { | ||
| test.beforeAll( async ( { requestUtils } ) => { | ||
| await requestUtils.activatePlugin( | ||
| 'gutenberg-test-inner-blocks-templates' |
There was a problem hiding this comment.
This test plugin seems to have been unused. Not sure how that happened. Probably a good chance to cull much of its code, but I won't do that in this PR.
I've pushed a commit that updates the test, and it should work as expected now. I think the main thing missing was saving the post, reloading and then checking the undo count and publish buttons aren't enabled. The other issue was that the test block was saving its inner blocks, which meant template sync wasn't being run. |
Mamaduka
reviewed
Nov 25, 2022
Comment on lines
35
to
36
| // The block template content appears asynchronously, so wait for it. | ||
| await blockWithTemplateContent.waitFor(); |
There was a problem hiding this comment.
Nit: I think this would be the same as the assertion below. I personally prefer more explicit assertions.
await expect( blockWithTemplateContent ).toBeVisible();
scruffian
approved these changes
Nov 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
The same issue appeared twice:
In both cases, we’re setting the template for a block asynchronously and that creates a persistent change in the post which seems to be because of how templated blocks work
This PR updates that to make sure applying templates does not create an undo level.
Why?
Applying templates to block inner contents should not create undo levels because by being automattic it is decent to assume the content is already saved and it will not depend on the initial state of the template.
How?
Using a simple
__unstableMarkNextChangeAsNotPersistentcall before applying the template.Testing Instructions
Easiest way to test is to checkout #45776 which implements this change.
Another way is to check out this PR and try to set block templates to custom blocks, core blocks, and also set the template async.
Screenshots or screencast
N/A