Use block context to detect presence of parent pattern for overrides #62861
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
talldan
added
[Type] Task
|
Size Change: -6 B (0%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ellatrix
approved these changes
Jun 26, 2024
|
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. |
| @@ -292,6 +280,8 @@ function shimAttributeSource( settings, name ) { | |||
| return { | |||
| ...settings, | |||
| edit: withBlockBindingSupport( settings.edit ), | |||
| // TODO - move this to be located with pattern overrides code. | |||
| usesContext: [ 'patternOverridesContent', ...settings?.usesContext ], | |||
There was a problem hiding this comment.
Should we move this to the usesContext property when registering the pattern overrides source?
There was a problem hiding this comment.
Will this also register usesContext for client side code?
Considering both pattern/overrides and patternOverridesContent are both sharing the same attribute via context, I guess they should be named the same thing. I'm not enamoured with using pattern/overrides naming in js code where the variable is often accessed via dot notation or destructuring.
I think changing it would require a backport.
There was a problem hiding this comment.
Will this also register usesContext for client side code?
If not, I think it could still be moved to this file if we change it over to the registerBlockType hook similar to the one in the use-binding-attributes.js file:
https://github.com/WordPress/gutenberg/blob/trunk/packages/editor/src/hooks/pattern-overrides.js#L110
There was a problem hiding this comment.
Will this also register usesContext for client side code?
Yes, it should work client-side. We might make some changes to how it is handled, but it should work.
I'm not familiar with how that context is used, but it'd be great to use just one property if possible.
I think changing it would require a backport.
Yes, I think it would need one. But it should be just one line.
There was a problem hiding this comment.
I decided to change it to pattern/overrides on the client as well, so that it's consistent and we won't need a backport.
I think I've addressed all the feedback.
|
I agree that using the context makes sense. Thanks for working on that! 🙂 I can see we have similar logic in the image edit file: link. Should we use context there as well? |
talldan
requested review from
artemiomorales and
michalczaplinski
as code owners
SantosGuillamot
approved these changes
Jun 26, 2024
SantosGuillamot
merged commit 9ee6ad3
into
fix/aspect-ratio-not-working-in-image-with-overrides
63 checks passed
SantosGuillamot
added a commit
that referenced
this pull request
Jun 27, 2024
…es (#62828) * Remove only supported attributes * Try alternative approach * Update comment * Return early while editing the original pattern * Move conditional inside conditional * Add missing check * Go back to running always bindings * Use block context to detect presence of parent pattern for overrides (#62861) * Use block context to detect presence of parent pattern * Regenerate fixtures * Update image block to use context for checking a parent pattern block exists * Rename context to `pattern/overrides` to be consistent with php code * Move pattern overrides context shim to pattern overrides hooks * Remove shim Co-authored-by: talldan <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> * Reduce diff * Change array order --------- Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: cbravobernal <[email protected]>
gutenbergplugin
pushed a commit
that referenced
this pull request
Jun 27, 2024
…es (#62828) * Remove only supported attributes * Try alternative approach * Update comment * Return early while editing the original pattern * Move conditional inside conditional * Add missing check * Go back to running always bindings * Use block context to detect presence of parent pattern for overrides (#62861) * Use block context to detect presence of parent pattern * Regenerate fixtures * Update image block to use context for checking a parent pattern block exists * Rename context to `pattern/overrides` to be consistent with php code * Move pattern overrides context shim to pattern overrides hooks * Remove shim Co-authored-by: talldan <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: SantosGuillamot <[email protected]> * Reduce diff * Change array order --------- Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: talldan <[email protected]> Co-authored-by: ellatrix <[email protected]> Co-authored-by: cbravobernal <[email protected]>
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 the branch from #62828, tries using block context to detect a parent pattern as an alternative to using selectors and traversing up the block tree.
Why?
Improved performance.
This also more closely matches the server implementation.
How?
providesContextto the pattern block's block.json, providing thecontentattributecontentattribute so that it's always truthy (the code can consistently check for a parent pattern using the attribute.usesContextfor the content attribute to all supported block binding blockscontextcheck.Testing Instructions
Follow the testing instructions in #62828