-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
InnerBlocks: combine store subscriptions #57032
Conversation
Size Change: +25 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in 552b908. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7211530682
|
} = props; | ||
|
||
useNestedSettingsUpdate( | ||
clientId, | ||
parentLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that all of these are internal sub hooks for only this component, so it's ok to change them.
58228f5
to
ba095aa
Compare
@@ -144,13 +144,13 @@ export function onBlockDrop( | |||
export function onFilesDrop( | |||
targetRootClientId, | |||
targetBlockIndex, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These functions should probably be rewritten a bit. There's some unused variables like this one, and I think other opportunities to use selector functions in callbacks instead of store subscriptions.
@@ -116,18 +123,22 @@ function UncontrolledInnerBlocks( props ) { | |||
|
|||
// This component needs to always be synchronous as it's the one changing | |||
// the async mode depending on the block selection. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel this commit is misplaced now no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it actually doesn't look relevant even in trunk. I think this was from before we created BlockListItems and used the async provider directly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep
name, | ||
blockType, | ||
innerBlocks, | ||
parentLock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that a public component? so we're adding props to a public component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or I guess it's the "controlled" one that is public and this is fine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, only useInnerBlocksProps
is public and a component wrapper around that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(this component is not even exported)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Who am I to argue with -20%
What?
This PR combines all the store subscriptions within InnerBlock into a single one. Note that the sub hooks are not public APIs but rather utility hooks of for InnerBlocks, so adjusting these is fine.
It also only runs the block context selector when needed.
Why?
First run: -21.5% type
Second run: -20.7% type
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast