Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jorgeart81
Copy link
Contributor

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

@@ -88,7 +90,7 @@ const PageLayout = ({ menu, breadcrumbs, children }) => {
<PageTitle breadcrumbs={breadcrumbs} />
</Toolbar>
<Divider />
{menu}
{menuMemo}
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Member

@tananaev tananaev Jul 2, 2024

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?

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants