Display children of inner block controllers in the block navigator#24083
Merged
noahtallen merged 3 commits intoAug 13, 2020
Merged
Conversation
noahtallen
requested review from
ajitbohra,
chrisvanpatten,
ellatrix,
mkaz and
youknowriad
as code owners
noahtallen
changed the title
|
Size Change: +62 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
youknowriad
reviewed
Jul 21, 2020
youknowriad
reviewed
Aug 12, 2020
Contributor
|
I don't like this selector argument tbh, but I can live with it :). I commented about a potential bug though. |
noahtallen
force-pushed
the
fix/block-navigator-not-showing-inner-block-controllers
branch
from
e29a327 to
64a40e8
Compare
Member
Author
|
here's the current look |
youknowriad
added
[Feature] Full Site Editing
[Type] Experimental
youknowriad
approved these changes
Aug 13, 2020
noahtallen
deleted the
fix/block-navigator-not-showing-inner-block-controllers
branch
6 tasks
Contributor
|
After some reports of typing performance regressions on 8.8 I used gitbisect and it seems that this PR introduced a big performance regression. |
youknowriad
reviewed
Aug 31, 2020
| const selectedBlockClientId = getSelectedBlockClientId(); | ||
| return { | ||
| rootBlocks: getBlocks(), | ||
| rootBlocks: getBlocks( '', { includeControlledInnerBlocks: true } ), |
Contributor
There was a problem hiding this comment.
Calling selectors with inline objects is never a good idea in terms of performance, not sure it's the source of the issue but it's definitely something to avoid in general (breaks memoization)
youknowriad
reviewed
Aug 31, 2020
| ? EMPTY_ARRAY | ||
| : getBlocks( state, clientId, { | ||
| includeControlledInnerBlocks, | ||
| } ), |
Contributor
There was a problem hiding this comment.
This call also breaks memoization as it will recompute getBlocks on each call since we're passing and inline object.
youknowriad
reviewed
Aug 31, 2020
| ( state, rootClientId, { includeControlledInnerBlocks = false } = {} ) => { | ||
| return map( getBlockOrder( state, rootClientId ), ( clientId ) => | ||
| getBlock( state, clientId ) | ||
| getBlock( state, clientId, { includeControlledInnerBlocks } ) |
Contributor
|
The good news is that this PR fixes the regressions #24835 |
Member
Author
|
Thank you for the tips about inline objects, I did not realize that would be the case! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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
Fixes #23459. In #21368, all "controlled" inner blocks were excluded from
getBlockandgetBlocks. This works because much of the time, you only want to know whhat blocks are in the particular entity you are looking at. For example, when you callgetBlockon a template part, you would not want to be aware of any children template parts. You only want to know which blocks are contained in that template part.However, there are still some bits of the UI (like the block navigator) which depend on seeing a list of all the blocks rendered on the page -- it does not care which blocks belong to which entity. So this PR:
getBlockandgetBlocks. Previously, those functions always excluded the children of nested inner block controllers. Now, these functions can include those nested children if a flag is set.BlockNavigationto include all blocks, including nested inner block controller children.How has this been tested?
Locally, in edit site.
Screenshots
Types of changes
Bug fix
Checklist: