Conversation
marekhrabe
left a comment
There was a problem hiding this comment.
I've tested this for regressions the same way I tested the previous implementation: I made a testing document to test this also outside the Navigation. Made Columns > Column > Cover > Paragraph and tried:
> currentBlock = wp.data.select('core/block-editor').getSelectedBlock().clientId
"203aef60-1cbc-466e-85a1-f98935188e53" // the deepest block - Paragraph
> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/cover')
["b5740327-7582-49c5-920e-01199b1fbbd7"] // correctly returned parent Cover
> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/columns')
["8086e200-37d5-4890-ab8d-880d587f17a8"] // correctly returned parent Columns
> wp.data.select('core/block-editor').getBlockParentsByBlockName(currentBlock, 'core/image')
[] // empty because there is no Image in parent list
Still getting expected results for both manual testing and the integration in Navigation
| ( { id } ) => id | ||
| ); | ||
| }, | ||
| ( state ) => [ state.blocks.parent ] |
There was a problem hiding this comment.
If I'm not mistaken we only care about state.blocks.parent[clientId]
There was a problem hiding this comment.
Maybe I misunderstood the dependant references here. I set state.blocks.parent because the selector depends on its parent, so if they change, it needs to clear the cache and regenerates.
However, it's already handled by getBlockParents() selector. Is this correct?
There was a problem hiding this comment.
Unless I'm wrong the returned value on change if the parents of a specific block (clientId) change, right?
There was a problem hiding this comment.
Actually, I'm wrong state.blocks.parent[ clientId ] doesn't exist, neighter state.blocks.parent. It should be state.blocks.parents as we can target all block parents at once.
There was a problem hiding this comment.
Beyond that .parent doesn't exist 🤦♂, is not already handled by the getBlockParents() selector?
oh, got it... we need to be totally specific about the dependant references. Since although it's true that the getBlockParents() selector cache depends on state.blocks.parents, if this selector depends just on the clientId, it's possible that it won't necessarily regenerate its cache if .parents tree changes.
getdave
left a comment
There was a problem hiding this comment.
Just noting that we're missing unit tests for this new selector.
f832fcd to
2a0cb59
Compare
It's a very valid point. Going to try to add it asap. |
It re implements the getBlockParentsByBlockName() selector using the createSelector() function to memoize what the selector returns in order to avoid performance issues. block-editor: update dependant references in getBlockParentsByBlockName() block-editor: fix
2a0cb59 to
f1aac92
Compare
Added unit tests for |
|
Thanks folks for the review. 🙇 |
Description
It re implements the getBlockParentsByBlockName() selector using the createSelector() function to memoize what t returns in order to avoid performance issues.
How has this been tested?
Manually
Create the following block structure
Visually, something like this:
or paste the following code in the Code Editor mode
When you inspect the nested Navigation Items, the colors should be propagated to all of them.
Also, you can check the behavior following the steps of this comment.
Unit tests
Checklist: