DRY up ContentBlocksList and BlockInspectorLockedBlocks #51281
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
Extract BlockQuickNavigation out of ContentBlocksList and BlockInspectorLockedBlocks so that we're not repeating the same code that outputs a list of block navigation links.
noisysocks
added
[Type] Code Quality
noisysocks
commented
Jun 7, 2023
| import BlockIcon from '../block-icon'; | ||
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| export default function BlockQuickNavigation( { clientIds } ) { |
There was a problem hiding this comment.
Not certain about what to name this component.
|
Size Change: -127 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
Mamaduka
reviewed
Jun 7, 2023
| getBlockName, | ||
| getBlockEditingMode, | ||
| } = unlock( select( blockEditorStore ) ); | ||
| return getClientIdsOfDescendants( [ topLevelLockedBlock ] ).filter( |
There was a problem hiding this comment.
This is a nice trick to get around the array/object referential equality "requirement" and avoid excessive re-renders 🥇
There was a problem hiding this comment.
Wait... it is? Won't filter return a new array every time? 😅
There was a problem hiding this comment.
You sent me down a rabbit hole of optimising all of this. I made some progress but will tackle it in a separate PR 😅
There was a problem hiding this comment.
Sorry about that 😅 I left two more comments in other PR, where returned values need optimization.
This map select returns one dimensional array of client IDs, correct?
There was a problem hiding this comment.
This comment? #50643 (comment) Yes I've been looking into that.
Yes this is a one dimensional array.
There was a problem hiding this comment.
I pushed 6006434 which reduces the number of re-renders in PagePanels and BlockQuickNavigation by selecting the minimal amount of data. I think BlockInspector here is as good as we can get it without creating a new selector which is not worth it right now imo.
I have more performance improvements ready but they're unrelated so they'll be in a separate PR 😀
|
I followed the steps and inserted the pattern. Selected it, but I still don't see the editable blocks in the right sidebar. Maybe I'm missing a step. Is there a specific way the pattern should be inserted? 
|
|
@apeatling: It should appear in the Block tab. Probably easiest to test patterns while editing a template or using the post editor. |
|
Flaky tests detected in e4421e8. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5207965918
|
apeatling
approved these changes
Jun 8, 2023
noisysocks
added
the
[Feature] Page Content Focus
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
) * DRY up ContentBlocksList and BlockInspectorLockedBlocks Extract BlockQuickNavigation out of ContentBlocksList and BlockInspectorLockedBlocks so that we're not repeating the same code that outputs a list of block navigation links. * Add back check that ensures list items don't appear in list * Remove unnecessary CSS * Reduce amount of rerendering * Update __experimentalGetGlobalBlocksByName
Mamaduka
reviewed
Aug 29, 2023
Comment on lines
+306
to
+312
| const blockNames = Array.isArray( blockName ) | ||
| ? blockName | ||
| : [ blockName ]; | ||
| const clientIds = getClientIdsWithDescendants( state ); | ||
| const foundBlocks = clientIds.filter( ( clientId ) => { | ||
| const block = state.blocks.byClientId.get( clientId ); | ||
| return block.name === blockName; | ||
| return blockNames.includes( block.name ); |
There was a problem hiding this comment.
@noisysocks, do you remember why you introduced this change? I can't find any place that passes the block names array to this selector.
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?
Follows #50857.
Extract a reusable component out of
ContentBlocksListandBlockInspectorLockedBlocksso that we're not repeating the same code that outputs a list of block navigation links.Why?
Less code good.
Testing Instructions
contentOnlylock. (Here's one.)