Paragraph: avoid re-rendering block controls on type #56797
Closed
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
|
Size Change: +141 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3e0aae6. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7112467161
|
ellatrix
changed the title
youknowriad
reviewed
Dec 6, 2023
| return align === ( isRTL() ? 'left' : 'right' ) || align === 'center'; | ||
| } | ||
|
|
||
| function Controls( { clientId, setAttributes } ) { |
There was a problem hiding this comment.
The main gotcha here is that this component doesn't depend on attributes and rely on useSelect to fetch the attributes instead.
I wonder if it's better to continue relying on attributes (because the wrapper will rerender anyway, but have three pure components (one of align, one for drop cap and one for rtl) which would avoid the useSelect
Now, I wonder about the feeling of over-optimization. So I'm kind of curious about the numbers of such refactor in general.
Also, I think we should draw some lesson from this PR:
- Either tell people to have "pure components" per control.
- Or normalize a controls.js + useSelect file per block.
I'm wondering also if the controls.js approach only works in the paragraph component because we don't access the "content" within the controls and that it might not be the case in all blocks which means the useSelect will be just an overhead instead of an optimization.
There was a problem hiding this comment.
Yes, it may be better for now to continue relying on the attributes as props, and only pass the needed ones. That's actually what I have done for now in #56813.
I don't think it's an over-optimisation if there's a clear difference in the numbers. Then most other performance PRs are over-optimisations. :D
Yes, as noted in the description, I do wonder what that means for the API. Tbh, I'm never been a big fan of having the controls and the content in the same Edit function. But yes, if we keep that it might be better to suggest making "pure" controls.
I don't think we access the content in many controls at all, if any at all.
There was a problem hiding this comment.
Actually, this PR is going to break "Block Bindings". Block Bindings work by updating the "attributes" prop that get injected into BlockEdit and this PR bypass that. So I'm thinking we should not merge it and instead do the granular controls approach.
There was a problem hiding this comment.
Can you explain this a bit more? What are "Block Bindings"? You mean mutating the attributes object?
There was a problem hiding this comment.
I don't find anything related to bindings for blocks in the codebase.
|
I've run the tests once more to check the consistency. |
|
Ok, huh, it seems like the results were wrong? 🤔 |
|
Well, if the difference is not that much, I agree it's not worth it. |
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?
This is an experiment inspired by #56783. While the color/typography panels no longer re-render, the controls added by the block itself are still being built on every keystroke, and when visible it's even worse, they will re-render (top toolbar enabled, or drop cap option shown). I also suspect registering the fills adds additional overhead, which happens even if the control is not visible.
I'm also wondering what this means for other block Edit functions. I don't really like that the content and controls are rendered together, but that ship has sailed. Together with a new block supports API, maybe we should rethink how controls are added though, also by blocks themselves.
Why?
After running the test, it seems there's about a 9.2% improvement (30.39 ms this branch vs 33.47 ms trunk).
Also "type in container" is better by 24% (8.36 ms vs 11.03ms) and "type without inspector" by 6% (30.17 ms vs 32.11 ms).
How?
By wrapping the controls in a separate, pure component (only re-renders from parent if the props change), subscribing itself to only the block attributes it needs.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast