-
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
Block editor: remove 4 useSelect in favour of context #56915
Conversation
Size Change: -82 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
Flaky tests detected in e0da783. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7165552576
|
a412e94
to
95710ab
Compare
|
||
export default function useBlockControlsFill( group, shareWithChildBlocks ) { | ||
const { isDisplayed, isParentDisplayed } = useDisplayBlockControls(); | ||
if ( isDisplayed ) { | ||
const { mayDisplayControls, mayDisplayParentControls } = |
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.
So these are two new APIs?
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 is internal context right?
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.
Oh, I guess it's not... We could lock it with symbols I guess?
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.
Fixed it 😊
c6ba25c
to
69c9d34
Compare
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.
Ok symbol as context keys as weird but if it's worth it in terms of performance, it seems fine. Maybe we should add a comment explaining the reasoning.
What?
#56847 has proven that the number of useSelect calls per block are definitely affecting performance. This PR removes another 4* of those separate store subscriptions, reuses the existing main block component subscription, and passes it down to slots and hooks through context (we already do that for clientId, name, etc.)
(*) Inspector slot, toolbar slot, block info slot, and one for hooks.
Why?
See above.
How?
Using context.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast