-
Notifications
You must be signed in to change notification settings - Fork 4.7k
Editor: Flatten Inserter mapSelectToProps to optimize rendering #11028
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
Conversation
| index: rootClientId ? getBlockOrder( rootClientId ).length : insertionPoint.index, | ||
| }, | ||
| layout: rootClientId ? layout : insertionPoint.layout, | ||
| index: rootClientId ? getBlockOrder( rootClientId ).length : insertionPoint.index, |
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.
The two lines above are different from what we had previously. As we were not assigning the insertionPoint.rootClientId before testing. Not sure if it's impactful.
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, you're totally right. I expect this is wrong as implemented. I'll plan to fix.
| const insertionPoint = getBlockInsertionPoint(); | ||
| const parentId = rootClientId || insertionPoint.rootClientId; | ||
|
|
||
| let index; |
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 think this should be initiailized something like getBlockOrder( rootClientId ).length?
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.
As far as I can tell, it's not needed, for reasons described in the comment a few lines below (maybe more valuable to pull up?)
// Otherwise, the default behavior for an undefined index is to
// append block to the end of the rootClientId context.
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.
Related:
gutenberg/packages/editor/src/store/reducer.js
Lines 422 to 428 in a548961
| const { index = subState.length } = action; | |
| return { | |
| ...state, | |
| ...mappedBlocks, | |
| [ rootClientId ]: insertAt( subState, mappedBlocks[ rootClientId ], index ), | |
| }; |
youknowriad
left a comment
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.
Feel free to merge when tests pass 👍
This pull request seeks to optimize the rendering of the
Insertercomponent by avoiding the return of a new reference object which had been returned previously in each call to itswithSelectmapSelectToProps. It does so by merely flattening theinsertionPointobject it had been creating.As a general rule of thumb,
mapSelectToPropsshould never create new objects used in values of its return object.This has the impact of reducing the number of render calls on
Inserterfor a new post from ~50 to 4.Testing instructions:
This is a refactoring change. There is expected to be no effective changes.
Ensure unit tests pass.
Verify basic usage of the inserter.