Remove pattern override experiment completely #58105
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
talldan
added
[Type] Task
talldan
requested review from
spacedmonkey,
kevin940726,
Mamaduka,
ellatrix,
artemiomorales,
michalczaplinski,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
talldan
changed the base branch from
trunk
to
update/individual-blocks-to-use-pattern-context
|
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/index.php ❔ lib/experimental/blocks.php ❔ lib/experimental/editor-settings.php ❔ lib/experiments-page.php |
talldan
commented
Jan 23, 2024
| withPartialSyncingControls | ||
| // Split into a separate component to avoid a store subscription | ||
| // on every block. | ||
| function ControlsWithStoreSubscription( props ) { |
There was a problem hiding this comment.
This is the only bit that I refactored a little bit, as otherwise it would've added a store subscription for every paragraph, heading, image and button. Now it'll only do so when one of those blocks is selected.
There was a problem hiding this comment.
it would've added a store subscription for every paragraph, heading, image and button.
Was that coming from a noticeable performance impact? My impression was that a store subscription should be cheap. But this is still a good refactor!
There was a problem hiding this comment.
Yep, apparently there have been good performance improvements from making these kind of changes, so this follows the trend: https://github.com/WordPress/gutenberg/pulls?q=is%3Apr+label%3A%22%5BType%5D+Performance%22+is%3Amerged+store+subscription+
|
Size Change: -341 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
talldan
force-pushed
the
update/individual-blocks-to-use-pattern-context
branch
from
aca9b90
to
cd183cc
Compare
talldan
force-pushed
the
remove/pattern-override-experiment-completely
branch
from
5cc3643
to
40a050e
Compare
kevin940726
reviewed
Jan 23, 2024
There was a problem hiding this comment.
I think we should restore this file to the upper level?
There was a problem hiding this comment.
Oh wait, I see what you mean.
| withPartialSyncingControls | ||
| // Split into a separate component to avoid a store subscription | ||
| // on every block. | ||
| function ControlsWithStoreSubscription( props ) { |
There was a problem hiding this comment.
it would've added a store subscription for every paragraph, heading, image and button.
Was that coming from a noticeable performance impact? My impression was that a store subscription should be cheap. But this is still a good refactor!
|
Flaky tests detected in 40a050e67d75cbf0e4617215968e2eb34ba4fd2b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7622221790
|
talldan
force-pushed
the
remove/pattern-override-experiment-completely
branch
from
9eda353
to
4109ef9
Compare
Base automatically changed from
update/individual-blocks-to-use-pattern-context
to
trunk
January 23, 2024 08:49
|
This pretty close now, but there are some puppeteer tests that need to be updated to account for pattern blocks no longer being directly editable. |
…sable block no longer uses controlled inner blocks)
kevin940726
force-pushed
the
remove/pattern-override-experiment-completely
branch
from
3b8e52c
to
61d48da
Compare
kevin940726
approved these changes
Jan 23, 2024
| } ); | ||
|
|
||
| // Check for regressions of https://github.com/WordPress/gutenberg/issues/33072. | ||
| it( 'can be saved when modified inside of a published post', async () => { |
There was a problem hiding this comment.
This was refactored into a playwright test below.
There was a problem hiding this comment.
Should we migrate tests from this file into various/patterns.spec.js, or does it make sense to keep a separate file?
P.S. I think various/patterns.spec.js should be blocks/patterns.spec.js.
There was a problem hiding this comment.
Should we migrate tests from this file into
various/patterns.spec.js, or does it make sense to keep a separate file?
I think there's no point in keeping separate files, TBH. Eventually we should migrate this file but that's for a follow-up 😅.
P.S. I think
various/patterns.spec.jsshould beblocks/patterns.spec.js.
Good point. No idea why the original test is in various? 😅
There was a problem hiding this comment.
Sounds good. I'm planning to handle the migration for this test file today.
There was a problem hiding this comment.
Thank you! We've done some changes to this block recently though (see #53705), so some parts might be outdated or confusing. Please reach out if you have any questions! Or feel free to punt it and I can give it a try maybe next week!
49 tasks
|
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 |
|
Thanks for flagging @getdave, I think the block bindings backport PR (WordPress/wordpress-develop#5888) takes care of any required backporting. From what I could tell there's no additional backporting for pattern overrides, as the rest of the implementation is in the block-library package (which is auto-backported). |
|
✅ I updated the PHP Sync Tracking Issue for WP 6.5 to note this PR does not require a backport for WP 6.5. The code here will be backported as part of WordPress/wordpress-develop#5888 |
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?
Builds on #58102 (ideally ship that PR first)
Dovetails with #58089
Removes the pattern overrides experiment flag completely.
The block bindings and custom field experiment flag will still need to be checked to use the pattern overrides feature
Why?
Aims to stabilize the feature ahead of the upcoming gutenberg and WordPress 6.5 release.
How?
Mostly just removes unused experiment flag code.
Testing Instructions