Change factor count calculation #967
Conversation
antoinebhs
force-pushed
the
change-factor-count-calculation
branch
from
e559374 to
a14074e
Compare
… into change-factor-count-calculation
| export interface FactorsCount { | ||
| resultCount: number; | ||
| variableCount: number; | ||
| } |
| ); | ||
| }; | ||
|
|
||
| export const useFactorCountDisplay = (count: number, maxCount: number, messageId: string, launchLoader: boolean) => { |
There was a problem hiding this comment.
launchLoader -> isLoading ?
I saw it's base on old code though, no need to change it here I think
There was a problem hiding this comment.
let's change it :)
| ); | ||
| }; | ||
|
|
||
| export const useFactorCountDisplay = (count: number, maxCount: number, messageId: string, launchLoader: boolean) => { |
There was a problem hiding this comment.
hook returning JSX ?
make this file a component instead
| ) | ||
| .map((entry) => entry[COUNT]) | ||
| .reduce((a, b) => a + b, 0); | ||
| const getFactorsCount = useCallback(() => { |
There was a problem hiding this comment.
| const getFactorsCount = useCallback(() => { | |
| const updateFactorCount = useCallback(() => { |
| const [sensitivityAnalysisParams, setSensitivityAnalysisParams] = useState(params); | ||
| const { snackError } = useSnackMessage(); | ||
| const [analysisComputeComplexity, setAnalysisComputeComplexity] = useState(0); | ||
| const [factorsCount, setFactorsCount] = useState<FactorsCount>({ resultCount: 0, variableCount: 0 }); |
There was a problem hiding this comment.
| const [factorsCount, setFactorsCount] = useState<FactorsCount>({ resultCount: 0, variableCount: 0 }); | |
| const [factorsCount, setFactorsCount] = useState<FactorsCount>(DEFAULT_FACTOR_COUNT); |
Then reuse it at line 169
| formatNewParams(filteredFormValues) | ||
| ) | ||
| .then((response) => { | ||
| response.text().then((value: string) => { |
There was a problem hiding this comment.
make this transformation in getSensitivityAnalysisFactorsCount directly to prevent promise chaining here
use backendFetchText ?
You could even use backendFetchJson so you don't have to parse it yourself
I'm confused that you need to parseInt yourself
There was a problem hiding this comment.
Fixed! I did not change the existing code (https://github.com/gridsuite/commons-ui/pull/967/changes#diff-17f4a437a70614bc66b84276022b6e01a917a2dba012ad66966df80c6d78128bL236-L242) but I really don't know why we went that way... Fixed in 6681389
| const timeoutId = setTimeout(() => { | ||
| setLaunchLoader(false); | ||
| }, 500); | ||
| return () => clearTimeout(timeoutId); |
There was a problem hiding this comment.
as discussed, this is weird, but we'll leave it since it was here before
There was a problem hiding this comment.
Agreed, probably need some ref/use effect to increase robustness...
antoinebhs
force-pushed
the
change-factor-count-calculation
branch
from
f184871 to
b18f0af
Compare
|
PR Summary