Pattern overrides: use block binding editing API #60721
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
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
changed the title
ellatrix
force-pushed
the
try/pattern-overrides-edit-binding
branch
from
d4531d6 to
15c2aba
Compare
22 tasks
|
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: |
talldan
reviewed
Apr 15, 2024
| 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
|
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! |
kevin940726
approved these changes
May 14, 2024
Oh yeah, |
SantosGuillamot
approved these changes
May 14, 2024
ellatrix
commented
May 14, 2024
| 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,
ellatrix
commented
May 14, 2024
|
|
||
| // 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 |
This was referenced Jun 3, 2024
Mamaduka
reviewed
Aug 21, 2024
| renderAppender: innerBlocks?.length | ||
| ? undefined | ||
| : InnerBlocks.ButtonBlockAppender, | ||
| value: innerBlocks.length > 0 ? innerBlocks : blocks, |
There was a problem hiding this comment.
@ellatrix, @SantosGuillamot, can you provide additional context around this change?
Use
blocksvariable untilinnerBlocksis populated, which has the proper clientIds.
What does proper clientIds mean here?
This seems to be the cause of the infinite loop regression in #63909.
There was a problem hiding this comment.
It's possibly related to the way useBlockSync clones the inner blocks for controlled blocks:
That gives the blocks new client ids.
Having new client ids is important. If you insert two instances of the same pattern into a post the inner blocks need unique client ids so that updates to one don't update the other.
There was a problem hiding this comment.
Sorry, I've been away for a few days 🙂
I remember having issues with having different clientIds in blocks and in innerBlocks as explained in this comment. I even refer to these lines there. It seems the innerBlocks clientIds were actually the ones in the markup, that's what I was trying to refer with "proper" clientIds.
What I don't remember is if I added that check because it was breaking something or just because I thought it was correct.
I'm fine trying to remove it and see if the tests pass.
Closed
2 tasks
This was referenced Sep 4, 2024
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?
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