Pattern overrides: use block binding editing API #60721
Conversation
ellatrix
added
[Type] Code Quality
ellatrix
requested review from
talldan,
artemiomorales,
kevin940726 and
SantosGuillamot
|
Size Change: -1.69 kB (-0.1%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
ellatrix
force-pushed
the
try/pattern-overrides-edit-binding
branch
2 times, most recently
from
c4b7073
to
d4531d6
Compare
ellatrix
changed the base branch from
add/support-for-editing-in-block-bindings
to
try/block-bindings-without-hooks
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
ellatrix
changed the base branch from
try/block-bindings-without-hooks
to
trunk
ellatrix
changed the base branch from
trunk
to
try/block-bindings-without-hooks
ellatrix
force-pushed
the
try/pattern-overrides-edit-binding
branch
from
d4531d6
to
15c2aba
Compare
|
I think this PR is intended to address #59817? Is the work @SantosGuillamot mentioned in the issue? There seem to be some failing e2e tests. Is this ready for review or still an experiment/WIP? |
|
This is ready for review, I just need to adjust the e2e tests. For some reason those blocks were added as inner blocks where there are actually controlled blocks from an entity? Not sure I understand why those tests are failing. |
|
I briefly tried it and noticed this thing: Screenity.video.-.Apr.15.2024.mp4The original content is lost as soon as the block allows overrides. I'm not sure if it's the expected behavior for other sources, but for pattern overrides, we want to leave the original content intact. Would it be something that needs to be special cased? |
Seeing a similar thing here, though additionally a few further issues with editing the original pattern. Any existing blocks in a saved pattern seem to be uneditable (e.g. create a pattern, save it, reload, try editing the original pattern content, the block is disabled and doesn't show up in List View). Blocks with bindings can be edited, but the edits aren't saveable. Not sure if it's more of an issue with the upstream branch. edit: The second issue might be related to this early return, perhaps should set the attribute on the block being edited when editing the source pattern: |
| const patternClientId = parents.find( | ||
| ( id ) => getBlockName( id ) === 'core/block' | ||
| ); | ||
| const blockName = currentBlockAttributes?.metadata?.name; |
There was a problem hiding this comment.
Nitpick - this line could be moved to after the early return.
ellatrix
force-pushed
the
try/block-bindings-without-hooks
branch
from
6fa790c
to
293b6df
Compare
| @@ -648,4 +702,205 @@ test.describe( 'Pattern Overrides', () => { | |||
| } ) | |||
| ).toBeHidden(); | |||
| } ); | |||
|
|
|||
| test( 'create a pattern with synced blocks', async ( { | |||
There was a problem hiding this comment.
We should probably mention "blocks with the same name" in the test title.
There was a problem hiding this comment.
I've changed the description as part of this commit. Let me know if that's fine 🙂
| let patternId; | ||
| const sharedName = 'Shared Name'; | ||
|
|
||
| await test.step( 'create a pattern with synced blocks', async () => { |
There was a problem hiding this comment.
I don't think we need this step. We already have tests for creating patterns with overrides and creating synced pattern, we don't have to repeat them here. This can be replaced with a requestUtils call:
const innerPattern = await requestUtils.createBlock( {
title: 'Blocks with the same name',
content: `<!-- wp:heading {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<h2>default name</h2>
<!-- /wp:heading -->
<!-- wp:paragraph {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<p>default content</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<p>default content</p>
<!-- /wp:paragraph -->`,
status: 'publish',
} );Also, it might be interesting to see when the default contents are different? 🤔
There was a problem hiding this comment.
I understood that when they share the name, we wanted to sync the values also while creating the original synced pattern, resulting in having the same default content. It'd be a bit confusing for me that the default content is not synced but whenever I use the pattern and update any of those values they become synced.
That's also why I added the two steps. The first one tests that it is synced while editing the original pattern, and the second tests that it is synced while using the pattern.
If that's not the case, we can revisit the implementation 🙂
There was a problem hiding this comment.
I see. Could we simplify this a little bit further then?
await requestUtils.createBlock( {
title: 'Blocks with the same name',
content: `<!-- wp:heading {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<h2>default name</h2>
<!-- /wp:heading -->
<!-- wp:paragraph {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<p>default content</p>
<!-- /wp:paragraph -->
<!-- wp:paragraph {"metadata":{"name":"${ sharedName }","bindings":{"content":{"source":"core/pattern-overrides"}}}} -->
<p>default content</p>
<!-- /wp:paragraph -->`,
status: 'publish',
} );
// Update the content of one of the blocks...
await headingBlock.fill('updated content');
// Check that every content has been updated.
for ( const block of [ headingBlock, firstParagraph, secondParagraph ] ) {
await expect( block ).toHaveContent( 'updated content' );
}There was a problem hiding this comment.
Side note: I personally still think the usage of this same name syncing feature is unknown. I don't know what the users should expect and why they would want to do this rather just creating nested patterns. 🤔
There was a problem hiding this comment.
Could we simplify this a little bit further then?
Sure, we can make that change 🙂
I personally still think the usage of this same name syncing feature is unknown.
I think it is a consequence of using the name as the identifier. If there are two blocks with the same name, how do you differentiate between them in the "content" object?
There was a problem hiding this comment.
I've simplified the approach as suggested in this commit. Let me know if that's not what you had in mind.
|
|
||
| // A Undo/Redo bug found when implementing and fixing https://github.com/WordPress/gutenberg/pull/60721. | ||
| // This could be merged into an existing test after we fully test it. | ||
| test( 'undo/redo should not lose focuses', async ( { |
There was a problem hiding this comment.
@kevin940726 This is not really an issue related to this PR, it's also broken for other inner block instances that are controlled. For example, try editing page or content inside the site editor. It will also lose focus on undo. This seems to be an issue that we need to address in controlled inner blocks.
There was a problem hiding this comment.
Nice find! I'm fine with removing it in this PR then 👍. Thanks!
There was a problem hiding this comment.
I've removed it in this commit. Feel free to add it again if you change your mind 🙂
|
I've been taking a deeper look, and it seems everything is working as expected, and tests are passing. Additionally, I believe the mentioned challenges seem to be solved. It'd be great to get a review to understand what is missing in order to merge this pull request. So far, I see two follow-up tasks:
Am I missing something? |
|
Everything is looking good to me! |
Oh yeah, |
| value: innerBlocks.length > 0 ? innerBlocks : blocks, | ||
| onInput: NOOP, | ||
| onChange: NOOP, | ||
| renderAppender: blocks?.length ? undefined : blocks.ButtonBlockAppender, |
There was a problem hiding this comment.
(blocks.ButtonBlockAppender will never exist)
There was a problem hiding this comment.
It seems that change has been there since the first commit, so I assume we just replaced innerBlocks by blocks. Maybe we should go back to the previous logic?
renderAppender: innerBlocks?.length
? undefined
: InnerBlocks.ButtonBlockAppender,
|
|
||
| // Check that the frontend shows the content of the pattern. | ||
| const postId = await editor.publishPost(); | ||
| await page.goto( `/?p=${ postId }` ); |
There was a problem hiding this comment.
Is there no preview util? This won't work with subdirectory installations, and breaks running tests in MAMP. We should honour the WP_BASE_URL.
There was a problem hiding this comment.
It seems there is a openPreviewPage util. I can try to use that in a follow-up PR.
|
Let's just create follow-up PRs |
|
So great to see this refactoring landed. Excellent collaboration! 🎉
One important aspect to keep in mind is that the current implementation of |
I agree. I plan to work on a follow-up PR for that, and we can discuss the best approach there. My idea is to change the current |
What?
Updates the pattern (
wp:block) block to use the block bindings API (from #60724) in the editor.Why?
This fixes a specific use case where two blocks (e.g. two paragraphs) may use the same names for the pattern overrides. In this case the two blocks should be synchronized when editing, but this is broken in
trunk.How?
Add new pattern overrides binding source properties for getValue and setValues
Testing Instructions
Expected: The two paragraphs can be synchronized.
Screenshots or screencast