-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
fix: useMemo in menus #1253
base: master
Are you sure you want to change the base?
fix: useMemo in menus #1253
Conversation
@@ -88,7 +90,7 @@ const PageLayout = ({ menu, breadcrumbs, children }) => { | |||
<PageTitle breadcrumbs={breadcrumbs} /> | |||
</Toolbar> | |||
<Divider /> | |||
{menu} | |||
{menuMemo} |
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.
Shouldn't the same thing be done in mobile layout?
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.
Also, how does it actually prevent re-rendering? Because the menu is still passed as a parameter every time. Have you verified that it actually works?
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.
It does not render again because in this case the useMemo has an empty dependency [ ].
I share the video of the tests.
Video
2024-07-02.00-48-07.mp4
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 menu won't be re-rendered, but the page will, no?
I'm not sure I understand what I'm supposed to see on the video, but I see re-rendering around 4th second.
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.
Yes, the page is rendered by the websocket because I have GPS connected to the traccar server.
The socket updates the state of the store, and this generates the render. Don't worry, it's part of React.
Version 19 is close and will bring performance improvements.
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.
But I don't understand what I'm supposed to see in the video.
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.
Previously all items in the report navigation menu were rendered with each socket update. With useMemo it no longer happens.
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 thought the problem was with re-rendering the map, not the menu. Why would re-rendering the menu a problem?
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 thought that was what you were referring to in the comment #1251 (comment)
I agree with you, menu rendering is not a problem.
Also thank you for your time, because these days I am understanding your platform better.
Prevent re-rendering of report and settings menus.
This solution would maintain the structure of your project and would not be as complex as @ #1252