Performance: Assign block list refs by constant reference #3171
Conversation
| @@ -65,21 +65,24 @@ class VisualEditorBlockList extends Component { | |||
| this.lastClientY = clientY; | |||
| } | |||
|
|
|||
| setBlockRef( ref, uid ) { | |||
| setBlockRef( ref ) { | |||
| const uid = ref.props.uid; | |||
There was a problem hiding this comment.
The only thing that bothers me about this, is that it's not obvious that we need to change this if we rename/remove the uid prop (or if React changes the element object shape)
There was a problem hiding this comment.
The only thing that bothers me about this, is that it's not obvious that we need to change this if we rename/remove the
uidprop (or if React changes the element object shape)
Sure, I can see this. I imagine if we did change the uid prop, there'd be much more impacted code than just what's referenced here. I don't love reaching into the element props, but don't necessarily see it as an unstable API.
How could this be improved? Would a comment help clarify?
I had considered removing the ref entirely and passing the node from the block component itself when selection starts, but this isn't sufficient since the block list needs to compare the selection with all rects of blocks in the list, so it needs access to every block's DOM node.
There was a problem hiding this comment.
Let's add a comment. Anyway nice improvement 👍
Creating new function reference would cascade render to VisualEditorBlock on every render
aduth
force-pushed
the
update/perf-block-list-ref
branch
from
a23ad07 to
68f6cbe
Compare
Codecov Report
@@ Coverage Diff @@
## master #3171 +/- ##
=========================================
- Coverage 31.51% 31.5% -0.01%
=========================================
Files 221 221
Lines 6305 6306 +1
Branches 1120 1120
=========================================
Hits 1987 1987
- Misses 3630 3631 +1
Partials 688 688
Continue to review full report at Codecov.
|
|
This introduced a regression where an error is logged when removing a block, since |
Related: #3170
This pull request seeks to optimize to eliminate a render cascade which occurs to every block when the VisualEditorBlockList component renders. Previously, we would assign a new function as the
refprop to every rendered VisualEditorBlock element, thereby incurring a new render to every block rendered in the post.The changes here seek to use a single constant reference of
setBlockRef, eliminating the customblockRefcomponent passed toVisualEditorBlockin favor offindDOMNodeto access the DOM node from the component instance.Testing instructions:
Verify there are no regressions in the behavior of block multi-selection.