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

Cleans up report-viewer.tsx #2498

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

Meklo
Copy link
Contributor

@Meklo Meklo commented Jan 15, 2025

Implements suggestions from comments by @jonenst in #2416 and additional improvements, here's a rundown :

  • Regroups selectedReportId and selectedReportType under a single state since they are tightly coupled
  • Reset reportTreeMap content before recomputing it to avoid possible incoherent state
  • Removes selectedReportId from callback dependency array by instead comparing it by its previous state
  • Consistently use useCallbacks from handlers sent to report-viewer's child components
  • Define a constant for container height offset
  • Remove conditional rendering since all were dependent on report which is required to instanciate report-viewer
  • Check for parentId value in onLogRowClick to avoid arbitrary string cast when value is possibly null
  • Removes useDispatch declaration which was unused

@Meklo Meklo requested a review from AbdelHedhili January 17, 2025 09:03
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.

1 participant