Fix nested pattern overrides and disable editing inner pattern #58541
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] Bug
kevin940726
commented
Feb 1, 2024
| @@ -309,7 +322,6 @@ export default function ReusableBlockEdit( { | |||
| }, [ syncDerivedUpdates, patternClientId, registry, setAttributes ] ); | |||
|
|
|||
| const handleEditOriginal = ( event ) => { | |||
| setBlockEditMode( setBlockEditingMode, innerBlocks, 'default' ); | |||
There was a problem hiding this comment.
This is removed because we no longer need it.
|
Size Change: +29 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
fix/nested-pattern-overrides
branch
from
e46fb28 to
b64dcde
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
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 Core SVNIf you're a Core Committer, use this list when committing to GitHub Merge commitsIf 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. |
kevin940726
force-pushed
the
fix/nested-pattern-overrides
branch
from
b64dcde to
29b11fa
Compare
|
I thought I posted a comment on this last week, but don't see it now 🤔 This is testing well for me in the editor, but changes to the nested pattern overrides do not show in the frontend for me still - but I think you might still be working on that. |
kevin940726
force-pushed
the
fix/nested-pattern-overrides
branch
from
29b11fa to
c15d464
Compare
|
@glendaviesnz Could you test it again with the latest WP core? I just tested it and the e2e test passed on CI as well. |
kevin940726
force-pushed
the
fix/nested-pattern-overrides
branch
from
c15d464 to
adfe948
Compare
talldan
approved these changes
Feb 6, 2024
There was a problem hiding this comment.
This is working really well for me, just a few small comments, but nothing major.
(edit: to confirm, I didn't have the issues @glendaviesnz noticed, and I used a fairly up-to-date verison of WordPress trunk)
| @@ -134,6 +135,7 @@ function getContentValuesFromInnerBlocks( blocks, defaultValues ) { | |||
| /** @type {Record<string, { values: Record<string, unknown>}>} */ | |||
| const content = {}; | |||
| for ( const block of blocks ) { | |||
| if ( block.name === blockName ) continue; | |||
There was a problem hiding this comment.
I think this check could be clearer to the reader (I had to scroll up to find where blockName comes from to understand what value it has).
Perhaps calling it patternBlockName or adding a comment above this code could help.
| @@ -250,7 +260,9 @@ export default function ReusableBlockEdit( { | |||
| // Apply the initial overrides from the pattern block to the inner blocks. | |||
| useEffect( () => { | |||
| defaultContent.current = {}; | |||
| const editingMode = getBlockEditingMode( patternClientId ); | |||
| const { getBlockEditingMode } = registry.select( blockEditorStore ); | |||
There was a problem hiding this comment.
Why use registry.select here over the alternatives?
It might be worth explaining it in a comment.
There was a problem hiding this comment.
It's basically the same thing as the alternative (using getBlockEditingMode from useSelect). I don't mind switching over just to make it easier to understand though!
There was a problem hiding this comment.
No real preference for me, just my curiosity.
kevin940726
force-pushed
the
fix/nested-pattern-overrides
branch
from
39f57c0 to
6807f0d
Compare
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?
Alternative to #58359. Fix #58291.
This PR does a few things:
The first two are the same with #58359. This PR also fixes the fourth but it's just a nice-to-have.
Why?
To prevent unexpected broken page. See #58291.
How?
Just some small refactorings.
Testing Instructions
Added an e2e test.
Screenshots or screencast