Block Editor: Optimize BlockListAppender#56116
Merged
Merged
Conversation
tyxla
added
[Type] Performance
tyxla
commented
Nov 14, 2023
Comment on lines
+92
to
+94
| if ( renderAppender === false ) { | ||
| return null; | ||
| } |
Member
Author
There was a problem hiding this comment.
What we're mostly doing is moving this condition a level above and moving the rest to a subcomponent. That way if the appender is false, we'll not have to create extra subscriptions because the subcomponent will never be rendered.
|
Size Change: +22 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
Member
|
Nice. I've also noticed this but was taking a different approach - returning early in But I think your approach is cleaner! I'll properly test the PR shortly. My version
diff --git packages/block-editor/src/components/block-list-appender/index.js packages/block-editor/src/components/block-list-appender/index.js
index 7b37b93d8b..5aab6200b5 100644
--- packages/block-editor/src/components/block-list-appender/index.js
+++ packages/block-editor/src/components/block-list-appender/index.js
@@ -42,6 +42,10 @@ function DefaultAppender( { rootClientId } ) {
function useAppender( rootClientId, CustomAppender ) {
const isVisible = useSelect(
( select ) => {
+ if ( CustomAppender === false ) {
+ return false;
+ }
+
const {
getTemplateLock,
getSelectedBlockClientId,
@@ -49,10 +53,6 @@ function useAppender( rootClientId, CustomAppender ) {
getBlockEditingMode,
} = select( blockEditorStore );
- if ( CustomAppender === false ) {
- return false;
- }
-
if ( ! CustomAppender ) {
const selectedBlockClientId = getSelectedBlockClientId();
const isParentSelected =
@@ -96,6 +96,10 @@ function BlockListAppender( {
const appender = useAppender( rootClientId, renderAppender );
const isDragOver = useSelect(
( select ) => {
+ if ( ! appender ) {
+ return false;
+ }
+
const {
getBlockInsertionPoint,
isBlockInsertionPointVisible,
@@ -111,7 +115,7 @@ function BlockListAppender( {
getBlockCount( rootClientId ) === 0
);
},
- [ rootClientId ]
+ [ rootClientId, appender ]
);
if ( ! appender ) { |
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
2 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.
What?
Similar to #56101.
This PR updates the
BlockListAppendercomponent to render the appender only if the appender component is not specifically set to befalse. In the case offalse, we never render it anyway, so with this change we'll not add unnecessary extra store subscriptions. The primary difference is that we're now doing that check outside of theuseSelectcall.Why?
This is an effort to minimize unnecessary store subscriptions created per block. See #54819 (comment).
Tested using @jsnajdr's debug code (c029303), the store subscriptions are reduced by almost 1k (~2%) on large text post with 1000 blocks.
Testing Instructions
Testing Instructions for Keyboard
No specific instructions.
Screenshots or screencast
None