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
Performance: Optimize the AddTemplate component used in data views pages #60586
Conversation
@@ -1,31 +1,431 @@ | |||
/** |
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've moved the old new-template file into this index file. There's no need to separate these two things and there's no need for props since this component is now used only for templates and only in the data views page.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
customGenericTemplate: 3, | ||
}; | ||
|
||
function NewTemplate() { |
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 believe we should split the modal into its own component. But it can be done separately.
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 I just went ahead and did this.
@@ -622,41 +624,57 @@ const useTemplatesToExclude = ( | |||
const useEntitiesInfo = ( | |||
entityName, | |||
templatePrefixes, | |||
additionalQueryParameters = {} | |||
additionalQueryParameters = EMPTY_OBJECT | |||
) => { |
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 was throwing a React warning because the useSelect was re-rendering the components while the data didn't change.
I just refactored it a bit to solve the above, but to be honest these hooks are very unclear, I'm not even sure we need them and why they exist as helpers. They just make things hard to follow.
4e7dbf1
to
55b8f11
Compare
Size Change: -425 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
@@ -58,3 +58,5 @@ export default function AddNewTemplatePart() { | |||
</> | |||
); | |||
} | |||
|
|||
export default memo( AddNewTemplatePart ); |
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.
Was there anything in this component that made the re-renders expensive?
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 actually was testing "templates" mainly, so this one is probably less expensive but I just thought that it doesn't really make sense to re-render this one when you're interacting with the data views. (like filtering...)
I think this PR improves the "navigate" metric (hard to tell on a single run because that metric is a bit unstable) |
What?
I was debugging the templates page and noticed that the "Add Template" component re-renders way too often. For instance, why is it re-rendering when I'm filtering the table.
I believe we can do better than this PR but at least this is a first step.
Testing Instructions
1- Open the templates page in the site editor
2- You can add a "console.log" to the AddTemplate component.
3- Notice that as you search, the component doesn't re-render.