Column Block: enable global template_lock inheritance #42677
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
t-hamano
added
[Type] Bug
|
Size Change: -74 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
t-hamano
changed the title
t-hamano
changed the title
|
The following e2e test verifies that it is possible to insert a block in a column block even when global template locking is set. gutenberg/packages/e2e-tests/specs/editor/plugins/cpt-locking.test.js Lines 122 to 138 in 5d5e97a |
t-hamano
changed the title
|
@jorgefilipecosta, might have more insight on this. |
There was a problem hiding this comment.
The only reason why the column had a templateLock set to false instead of inheriting was that in the past, columns had a template lock of false. Now columns do not set a templateLock so I guess this change makes sense.
We need to update the test case and after that, I think this PR can be merged.
Thank you for this PR @t-hamano 👍
t-hamano
commented
Sep 2, 2022
| @@ -119,22 +119,22 @@ describe( 'cpt locking', () => { | |||
| ); | |||
| } ); | |||
|
|
|||
| it( 'can use the global inserter in inner blocks', async () => { | |||
| it( 'should not allow blocks to be inserted in inner blocks', async () => { | |||
There was a problem hiding this comment.
I think the name of this test is more easier to understand.
t-hamano
commented
Sep 2, 2022
| await pressKeyTimes( 'Tab', 2 ); | ||
| await page.keyboard.press( 'Enter' ); | ||
| expect( await page.$( '.wp-block-gallery' ) ).not.toBeNull(); | ||
| expect( |
There was a problem hiding this comment.
This test expects that the inserter in the column block is not available.
t-hamano
commented
Sep 2, 2022
| ) | ||
| ).toBeNull(); | ||
|
|
||
| expect( |
There was a problem hiding this comment.
I have added a test to confirm that the global inserter is also disabled.
This code is the same as what we do in shouldDisableTheInserter, but for the sake of clarity, I have copied the code inside verbatim. Without the use of copy, I think it would be difficult to understand what you are trying to do as follows:
it( 'should not allow blocks to be inserted in inner blocks', async () => {
await page.click( 'button[aria-label="Two columns; equal split"]' );
expect(
await page.$(
'.wp-block-column .block-editor-button-block-appender'
)
).toBeNull();
// Hard to know if we are actually testing or trying to perform some action.
shouldDisableTheInserter();
} );
jorgefilipecosta
added
the
Needs Dev Note
jorgefilipecosta
approved these changes
Sep 6, 2022
There was a problem hiding this comment.
Thank you for this PR @t-hamano it is ready to merge 👍
It may have some impact, previously a column was always unlocked, and inheritance was not consistent with the other blocks, and now it is. Some blocks/users may be affected so I guess it worths a dev note as part of the next WordPress release https://make.wordpress.org/core/handbook/tutorials/writing-developer-notes/. Would you be able to include a short note as a comment of this PR ? That note should be published as part of the next WordPress release dev notes post. Thank you in advance!
|
@jorgefilipecosta |
5 tasks
|
This is a comment about dev note. The column block did not inherit the global template_lock for post types. |
|
@t-hamano I am a bit confused. Maybe that's because the dev note comments is a bit short? @jorgefilipecosta You mention |
|
@bph I have added this PR to the "Block Library (Package)" section of 6.1 Dev Notes Tracking. I think this PR should be included in a dev note about the block library as well as #43663, but I think there is no overall dev note yet. I would be happy if someone could set up an overall dev note, as I'm not good at English and writing 🙏 |
Hi @bph, no, I don't have a dev notes post in progress. Normally there is a dev notes post that contains many small changes. I guess we can include this note in one of the posts that refers to multiple changes. |
|
Moved the discussion to the 6.1 Dev Notes issue so it's not further fragmented over multiple PRs. |
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.
Issues to be fixed:
Related PRs:
What?
This PR fixes a problem that template lock doesn't work correctly in
core/columnblock.As per the above issue and PR, this issue has been recognized for a long time, and I believe the discussion was centered on "whether to let column blocks inherit template_lock".
However, compared to the past, template locks and block locks have been enhanced.
I think it is reasonable now to allow column block to inherit
templateLockin order to unify the behavior of the lock function.Why?
I confirmed that only
core/columnblock has the default value oftemplateLockset to false as attirbute.gutenberg/packages/block-library/src/column/edit.js
Line 31 in 5cca6b6
If I understand correctly,
templateLockas attribute isundefinediftemplate_lockis enabled throughout the editor.Therefore, I think that the default value of
templateLockforcore/columnblock wasfalseand applied to innnerBlocks.In particular, a critical bug that is caused by template_lock not being inherited such as #37912 should be fixed.
How?
Simply removed default values.
Testing Instructions
template_lockwith the following code:Please see this comment for more information on the defects this PR is trying to fix.