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

Implement custom report tree for report viewer #2416

Merged
merged 24 commits into from
Dec 27, 2024
Merged

Conversation

Meklo
Copy link
Contributor

@Meklo Meklo commented Nov 25, 2024

No description provided.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has totally checked 677 files.

Valid Invalid Ignored Fixed
633 2 42 0
Click to see the invalid file list
  • src/components/custom-treeview/TreeViewItem.tsx
  • src/components/custom-treeview/VirtualizedTreeView.tsx

src/components/custom-treeview/TreeViewItem.tsx Outdated Show resolved Hide resolved
src/components/custom-treeview/VirtualizedTreeView.tsx Outdated Show resolved Hide resolved
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

license-eye has totally checked 685 files.

Valid Invalid Ignored Fixed
640 1 44 0
Click to see the invalid file list
  • src/components/custom-treeview/use-treeview-scroll.ts

src/components/custom-treeview/use-treeview-scroll.ts Outdated Show resolved Hide resolved
@Meklo Meklo changed the title Implement custom report tree for report viewer [POC] Implement custom report tree for report viewer Nov 28, 2024
@Meklo Meklo changed the title [POC] Implement custom report tree for report viewer Implement custom report tree for report viewer Dec 18, 2024
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
Signed-off-by: Joris Mancini <joris.mancini_externe@rte-france.com>
</ReportItem>
);
const mapReportsById = useCallback((item: ReportTree) => {
reportTreeMap.current[item.id] = item;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should maybe throw away the old map and assign a new empty object to reportTreeMap.current ?

setSelectedReportType(reportTreeMap.current[report.id].type);
}
},
[selectedReportId]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe in a separate PR we can try to refactor this code a bit. I find it weird for example that we go to the extend of using a ref for the map of ids to reports to avoid having it in the dependencies of this callback, but at the same time it has selectedReportId in its dependencies which presumably changes more often than the reportTree (and maybe even always changes when the reporttree is changed?).
Another weird thing is top use useCallback for callbacks for VirtualizedTreeview, but not for callbacks for LogTable (onLogRowClick is not a useCallback).. To be cleaned up later I guess

@ayolab ayolab self-requested a review December 26, 2024 08:45
@ayolab ayolab merged commit 518f068 into main Dec 27, 2024
5 checks passed
@ayolab ayolab deleted the feat/custom-report-tree branch December 27, 2024 08:48
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.

4 participants