Repository navigation
Fix/prevent re-render reports menu - #1252
jorgeart81 wants to merge 15 commits into
Conversation
| const ReportsLayout = () => ( | ||
| <PageLayout | ||
| menu={<ReportsMenu />} | ||
| breadcrumbs={['reportTitle', 'reportCombined']} |
There was a problem hiding this comment.
The breadcrumbs is different per report.
| </Route> | ||
|
|
||
| <Route path="reports"> | ||
| <Route path="reports" element={<ReportsLayout />}> |
There was a problem hiding this comment.
Should we do the same thing for settings?
There was a problem hiding this comment.
It seems to me that yes, in the components that have the same design.
It would be necessary to analyze it because I do not see that the state update generated by the socket is rendering the settings menu.
I apologize for any translation errors, my language is Spanish and I am using a translator.
There was a problem hiding this comment.
Can you please update settings as well, so we have consistent codebase.
There was a problem hiding this comment.
Of course, let me see what I can do.
|
Can you please clarify how this fixes the problem. |
|
Response to comment #1252 (comment) Child components are affected by the rendering of the parent or container component. |
| '/reports/combined': ['reportTitle', 'reportCombined'], | ||
| '/reports/route': ['reportTitle', 'reportRoute'], | ||
| '/reports/event': ['reportTitle', 'reportEvents'], | ||
| '/reports/trip': ['reportTitle', 'reportTrips'], | ||
| '/reports/stop': ['reportTitle', 'reportStops'], | ||
| '/reports/summary': ['reportTitle', 'reportSummary'], | ||
| '/reports/chart': ['reportTitle', 'reportChart'], | ||
| '/reports/logs': ['reportTitle', 'statisticsTitle'], | ||
| '/reports/scheduled': ['settingsTitle', 'reportScheduled'], | ||
| '/reports/statistics': ['reportTitle', 'statisticsTitle'], |
There was a problem hiding this comment.
You probably don't need to duplicate 'reportTitle' everywhere. Just the second part is different.
There was a problem hiding this comment.
I made this change, what do you think? 395ada3
There was a problem hiding this comment.
I found that being part was different-> '/reports/scheduled': ['settingsTitle', 'reportScheduled'],
traccar-web/src/reports/ScheduledPage.jsx
Line 65 in 66b964c
There was a problem hiding this comment.
This is probably a mistake. All of them should be under reports.
| import ReportsMenu from './ReportsMenu'; | ||
|
|
||
| const routes = { | ||
| reports: { |
There was a problem hiding this comment.
Why do we need a nested JSON here. I think reports property should be removed. This is only reports anyway.
| const commondBreadcrumb = 'settingsTitle'; | ||
|
|
||
| const routes = { | ||
| settings: { |
| }, | ||
| }; | ||
|
|
||
| const compoundRoutes = { |
There was a problem hiding this comment.
This looks really complicated. Why do we need all of these? I think you should be able to split it by path segements.
There was a problem hiding this comment.
What do you think of this alternative?
https://gist.github.com/jorgeart81/6140d1afeab31ae690b11eedf8dca106
There was a problem hiding this comment.
Seems like it's still overcomplicated. Why can't we just append to an array? Something like:
let breadcrumbs = ['settingsTitle'];
if (length >= 2) {
breadcrumbs.add(routes[pathSegmets[1]);
}
if (length >= 3) {
switch ...
}
There was a problem hiding this comment.
There was a problem hiding this comment.
The other PR looks promising, if it works as expected. Commented there.
|
We already have a solution for this. We only re-render when id or name changes. |
2 participants
Create a ReportLayout so that the ReportsMenu is not affected by the status update.
Ref -> PR/1251
Video
2024-06-28.18-54-18.mp4