-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat: dont simulate if there are no plots or secondary params #574
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## ui-enhancements #574 +/- ##
================================================
Coverage 76.17% 76.17%
================================================
Files 108 108
Lines 5600 5600
================================================
Hits 4266 4266
Misses 1334 1334 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
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.
LGTM! I think there's a useEffect
that you can remove. Maybe run yarn format
if you haven't already got VSCode set up to run Prettier when you save changes.
@@ -17,8 +17,11 @@ import UnitField from "../../components/UnitField"; | |||
import SimulationSliderView from "./SimulationSliderView"; | |||
import HelpButton from "../../components/HelpButton"; | |||
import { ExpandLess, ExpandMore } from "@mui/icons-material"; | |||
import { ChangeEvent, useState } from "react"; | |||
import { ChangeEvent, useEffect, useState } from "react"; |
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 don't think this component uses any effect hooks.
@@ -70,6 +76,7 @@ export default function useSimulation( | |||
simulate, | |||
JSON.stringify(simInputs), | |||
page, | |||
runSimulation, | |||
]); |
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.
At some point, I think I should rewrite this hook, to see if I can reduce the number of dependencies for this effect.
I'll wait for this to merge before merging #575, to avoid conflicts. |
Quality Gate passed for 'pkpdapp'Issues Measures |
Quality Gate failed for 'pkpdapp-team_pkpdapp_frontend'Failed conditions |
No description provided.