Try changing setBlockAttributes to update dynamicContent
#54233
Closed
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
kevin940726
added
[Type] Experimental
|
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/block-supports/pattern.php |
|
Size Change: -646 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Great job progressing this exploration @kevin940726 👍
Overall it tests pretty well. Mostly just the same issues as the original proof of concept has.
For example:
- The first edit of an inner block requires the pattern entity to be saved
- Needing to lock blocks etc
One thing I did notice comparing this branch to #53887, was that if I added pattern support to a button block for its text and url attributes, this branch loses the text customization when setting the URL.
It should be solvable once we can devote time to this again.
Screen.Recording.2023-09-07.at.5.08.57.pm.mp4
gziolo
reviewed
Oct 3, 2023
There was a problem hiding this comment.
It is an excellent idea to tap into the block editor's store to add support to dynamic content there directly. I left a comment that it would be great to follow up with attributes that might be more challenging due to a higher impact on the performance.
Once we have that in place, and the feedback from @aaronrobertshaw is included, we can start working on integrating the store with Block Binding API. It looks like you can land the part concerning the pattern separately. I need to discuss it further with @SantosGuillamot, but it feels like we are getting closer with every prototype to the expected state.
| } | ||
| { ...props } | ||
| /> | ||
| <BlockEdit { ...props } attributes={ sourceAttributes } /> |
There was a problem hiding this comment.
This is interesting. I wonder if the override for attributes should also happen on the store level, similar to how modifications get applied to updateBlockAttributes. Otherwise, we risk that folks could omit using attributes passed to Edit and get local values for dynamic content when accessing the store directly similar to what would happen when they use updateBlockAttributes.
gziolo
added
the
[Feature] Block API
|
One additional note, in #54536 (comment) @SantosGuillamot proposed to cover the following core blocks in the initial scope:
I believe it would be at least the following:
|
16 tasks
talldan
reviewed
Oct 5, 2023
Comment on lines
+177
to
+192
| const parentBlocks = select.getBlocksByClientId( | ||
| select.getBlockParents( clientId ) | ||
| ); | ||
| const parentPattern = parentBlocks.findLast( | ||
| ( parentBlock ) => parentBlock.name === 'core/block' | ||
| ); | ||
| const block = select.getBlock( clientId ); | ||
| if ( | ||
| ! parentPattern || | ||
| ! registry | ||
| .select( blocksStore ) | ||
| .hasBlockSupport( block.name, '__experimentalPattern' ) | ||
| ) { | ||
| updates[ clientId ] = attrs; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
I like the explorations you and @aaronrobertshaw have worked on!
Given the block bindings API is going to support a bunch of different data sources, I wonder how this can be made generic/pluggable, and done in a way where the core/block isn't hard-coded in.
Having read through the block bindings issue, it seems like the blocks inside the pattern will have some config denoting their connections (which I guess the pattern creator will define), which could be read here. Then there's the need to discover the source of the data for those connections, which is what happens here but somewhat hard-coded.
I think maybe we're close to the point where we can try modelling those connections and abstracting the discovery of the data source that happens here. It could be that the connection 'type' determines the strategy used to find the data source (the pattern block). Or alternatively the pattern block is somehow defined as source.
But we should at the same time connect back with the other data bindings work around custom fields to make sure the approach taken works in both places. Looking at the POCs so far for custom fields (#53300), the challenge is that they focus more on the PHP part as it stands.
There was a problem hiding this comment.
Yeah, exactly which blocks and their attributes should be connected was still undecided. This part was just following the existing exploration in the base branch.
The main goal for this PR is to find a performant and extendible way to allow override of setAttributes. Writing condition paths directly in updateBlockAttributes works for prototyping, but I'm currently trying to find a better approach so that it feels less like "duct-taping". I've tried maybe 6 different approaches now but each has its trade-offs and challenges. Once we solve that, and we have the connection "contract" defined, then I think we'll be in a good shape for implementing them.
|
This is superseded by #56235. |
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?
Based on #53887. This PR tries changing
setBlockAttributesto a thunk and updating the parent pattern'sdynamicContentattribute.Why?
The original approach monkey-patch
setAttributesfor theBlockEditcomponent. This works when the code directly callssetAttributesfrom the props, but not when it dispatchessetBlockAttributesfrom the store.How?
Converting the
setBlockAttriutesaction into a thunk and performing logic there.This PR also removes the need to store the
patternIdattribute on the pattern block, because we can just walk up the tree to find the immediate parent pattern for a given block. This also removes theuseEffectused to mark the pattern withpatternIdon mount.Testing Instructions
Enable the experimental gutenberg flag in Gutenberg -> Experiments -> Test partially synced patterns and follow the instructions in #53887.
Screenshots or screencast
Kapture.2023-09-07.at.10.33.57.mp4