Block Editor: Check all ancestors inside canInsertBlockTypeUnmemoized selector
#39228
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
|
Size Change: +18 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
DAreRodz
changed the title
canInsertBlockTypeUnmemoized selectorcanInsertBlockTypeUnmemoized selector
|
@DAreRodz, could you please check which of these blocks stop working if the parent is not direct (i.e.: we break flexbox)?
|
ntsekouras
reviewed
Mar 6, 2022
There was a problem hiding this comment.
Thanks for the PR @DAreRodz!
While this seems like something missing - which might actually that be the case - this API exists for some time and its restriction might be something that other blocks(core or third party ones) might rely on. An example on top of my mind is the Column block.
This will effectively change the current(expected) behavior and we might want to think about it a bit more and certainly test well because it's more convoluted than it seems.
There were discussion about revisiting the similar 'restriction' APIs like parent, allowedBlocks, allowedBlockTypes, etc..
|
I just wanted to echo what @ntsekouras raised. It's a very tempting change but it completely changes the behavior. It's called a parent for a reason because it creates a direct parent-child relation. In practice, in most cases having a way to define an ancestor would work better, but it's a different use case in which we would have to encode explicitly in a separate API to keep backward compatibility. To continue on the example on what Nik shared, in the case of the Columns block, at the moment you can put a Column only as a direct child of Columns:
With the change proposed it would be possible to insert blocks as follows:
The latter would be incorrect. |
|
Hey, thanks for your fast feedback @luisherranz, @ntsekouras, @gziolo. It totally makes sense, you could mess everything by moving blocks that are supposed to be right under specific blocks, like Column under Columns. I just created this PR because I did a quick test, and I saw it was easy to make it work, but maybe it would have been better to open an issue first for having this discussion. 😅 Let’s continue the conversation in the related issue, and feel free to close this PR. 😊 |
Awesome. Thank you for starting an issue. This PR is helpful as it can drive the development of the new API for ancestor blocks 👍🏻 |
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.
Description
This PR introduces minor changes inside
canInsertBlockTypeUnmemoizedto check if any of all the block's ancestors match any of the block types specified in itsparentattribute (insideblock.json), returningtrueif it's the case.The change would allow blocks to be inserted inside any block, as long as one of the ancestors' type is included in
parent.Although the purpose of this PR is to solve #37181, which is related to comment blocks, I think it could be useful in more cases. For context, see #37181 (comment).
Testing Instructions
Checklist:
*.native.jsfiles for terms that need renaming or removal).