Use block naming for marking blocks as overridable in patterns #59268
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] Enhancement
talldan
requested review from
youknowriad,
mtias,
gziolo,
artemiomorales,
michalczaplinski,
kevin940726 and
SantosGuillamot
|
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/compat/wordpress-6.5/block-bindings/pattern-overrides.php |
|
Size Change: +1.73 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in eb5bc0f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8001601005
|
|
Hi Dan! 👋 Excuse my ignorance - could you explain briefly why using the block metadata instead of nano ids might be a better idea? |
|
@michalczaplinski There's been some discussion on the issue about it, starting from this point - #53705 (comment). And @youknowriad asked me to look into this further, as he's concerned there might be some missed opportunities (around the possibility for AI integrations in WordPress, and also shuffling of patterns) for the future if the pattern block doesn't store the correct data from the start. So I agreed to look into it, even though it is very late in the WordPress release cycle. (and sorry for the lack of info, I put this together pretty quickly in an attempt to get early feedback) |
talldan
added
the
Backport to WP 6.7 Beta/RC
youknowriad
force-pushed
the
try/block-name-instead-of-nanoid-for-pattern-overrides
branch
from
b667034 to
e11393c
Compare
|
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. |
…de bindings - Remove 'Allow instance overrides' checkbox - In its place, add a hook that detects whether a metadata name is set, and use that to create bindings - Update the PHP binding callback to use the block metadata name
…of `content` attribute
There was a problem hiding this comment.
Thanks for getting the ball rolling on this @talldan 👍
I'm still getting up to speed on the latest with pattern overrides so please take this review with a grain of salt. My plan is to dig a bit deeper over the next couple of days and test more thoroughly.
While testing the happy path, I think I might have hit a small issue. If a block is named via the list view, it doesn't seem to be picked up as being overridable. If the block is named via the Block Name field within the block inspector's Advanced panel, it does show as overrideable.
Here's what I was seeing:
Screen.Recording.2024-02-28.at.5.17.59.pm.mp4
To rely on the act of naming a block as indication it is eligible for overriding, we might need to update the list view renaming of blocks as well.
Other than that I only have some really minor nits after a first pass over the code.
Nice work and impressive turnaround time 🚀
talldan
force-pushed
the
try/block-name-instead-of-nanoid-for-pattern-overrides
branch
from
e11393c to
ccf4ece
Compare
talldan
force-pushed
the
try/block-name-instead-of-nanoid-for-pattern-overrides
branch
from
ccf4ece to
fa3bb13
Compare
|
Thanks for addressing the linting issues @aaronrobertshaw ❤️ I've pushed a commits that updates the e2e tests. They also caught a bug with the individual 'reset' button (that hadn't been updated to use the names), which I've fixed. Loving those e2e tests 😄 |
It'd also be a great way to fix this issue - Pattern override: individual binding attributes are saved in the pattern markup and don't auto-update, so something that can be explored alongside expanding block and attribute support for bindings. |
talldan
changed the title
kevin940726
reviewed
Mar 1, 2024
aaronrobertshaw
approved these changes
Mar 1, 2024
There was a problem hiding this comment.
Thanks for adding the new deprecation fixtures 👍
I think we're probably still a fixture short, that being one for the latest version of the block with overrides e.g. core__block__overrides.html. That doesn't have to block this though and can be a follow-up.
✅ Fixture tests pass.
✅ Tests as advertised still
LGTM.
youknowriad
deleted the
try/block-name-instead-of-nanoid-for-pattern-overrides
branch
Good point! edit: added the fixture in #59492 |
|
Here's the trac ticket for the backport - https://core.trac.wordpress.org/ticket/60665 |
49 tasks
youknowriad
added a commit
that referenced
this pull request
Mar 4, 2024
Co-authored-by: talldan <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: michalczaplinski <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: gziolo <[email protected]>
|
I just cherry-picked this PR to the update/packages-6.5-rc1 branch to get it included in the next release: 9332cee |
youknowriad
added
Backported to WP Core
youknowriad
added a commit
that referenced
this pull request
Mar 4, 2024
Co-authored-by: talldan <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: kevin940726 <[email protected]> Co-authored-by: michalczaplinski <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: gziolo <[email protected]>
This was referenced Mar 13, 2024
22 tasks
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?
Attempts to switch over to using the block metadata name instead of a nano id as the connecting glue for binding pattern overrides.
How?
PartialSyncingControlscomponent with a hook that monitors whether a metadata name for a block has been entered and appropriately adds/removes the bindings for blocksvaluessub-property in the pattern blockcontentattributeCurrent issues / questions
Testing Instructions
Backwards compatibility testing
trunkchecked out and name the pattern something like 'Old pattern' 😄Screenshots or screencast