Use a patch format and support linkTarget of core/button for Pattern Overrides
#58165
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] Enhancement
kevin940726
requested review from
talldan,
glendaviesnz,
artemiomorales,
michalczaplinski and
SantosGuillamot
kevin940726
requested review from
spacedmonkey,
Mamaduka and
ajitbohra
as code owners
|
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/experimental/block-bindings/sources/pattern.php ❔ lib/experimental/blocks.php |
|
Size Change: +73 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
49 tasks
kevin940726
changed the title
linkTarget of core/button for Pattern Overrides
kevin940726
force-pushed
the
add/pattern-overrides-patch-format
branch
from
84d0541
to
0f31eaf
Compare
|
Flaky tests detected in 0f31eaf. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7638310239
|
draganescu
approved these changes
Jan 24, 2024
There was a problem hiding this comment.
This change is great IMO, the idea to represent overrides as a patch on attributes as JSON data opens up a more structured way to rerepsent operations which overrides will require.
@kevin940726 might as well fix the rel nofollow setting as well in this PR?
I tested it and it fixes the issue.
|
I noticed this PR was merged after I raised the PHP Sync Tracking Issue for WP 6.5 and has changed PHP files that may need backporting to WP Core. Please forgive the ping, but I marked as |
|
I personally don't think the format this PR introduces is something to ship in 6.5. This PR was merged without much feedback. |
|
@kevin940726 @draganescu It looks like this API doesn't have full support from all contributors. I'd be grateful if you could discuss and advise as to whether you will be looking to include this PR in WP 6.5. Very much appreciated 🙇 |
Maybe pattern overrides can ship without support for |
|
I did not mean to imply this should be shipped in core just because it's merged. If there is a need to debate the concept more let's revert. I kinda agree with @SantosGuillamot that instead of doing something else naybe let's not handle |
Block Bindings API is general enough that it doesn't enforce any format, so it should be perfectly fine to use the approach used in this PR. However, I share the concern about making such a decision so early based on a single use case shaped by the |
|
I agree with @gziolo that it might be too early to ship this for the one use case. There are also a few other issues cropping up with block bindings/pattern overrides (like #58425) that the patch format doesn't solve for, so that makes me feel like it isn't the right time. I'm also not sure that the patch format was what fixed this issue, it seems like it was more using an empty string for the attribute value? |
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5. It will be reverted in #58489. |
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?
Part of #53705. An alternative to #57993 without the Block Binding API changes.
Supports the
linkTargetattribute of thecore/buttonblock when unchecking "Open in new tab".Why?
See #57249 (comment) and #57993. To minimize the breaking changes, we decided that using a patch format is more flexible and "future-proof".
How?
Use a patch format similar to JSON patch to represent overriding an attribute.
We use tuples and integers to minimize the size saved in the serialized string. Other operations (like
add) and the third element of the tuple are reserved for future uses (e.g. support only overriding a partial path of an object).Currently, we use an empty string for the
pattern_attributesbinding to "unset" the attribute. This is intentionally kept ambiguous until the block binding API supports different operations.Testing Instructions
Follow the instructions in #53705 (comment) to create a pattern with overrides.
Screenshots or screencast
Kapture.2024-01-24.at.17.39.46.mp4
(My browser was slow in the video for unrelated reasons)