fix(gauge): value arc angle computation #494
Conversation
Codecov Report
@@ Coverage Diff @@
## main #494 +/- ##
=======================================
Coverage 85.60% 85.60%
=======================================
Files 757 757
Lines 15518 15519 +1
Branches 1839 1839
=======================================
+ Hits 13284 13285 +1
Misses 2203 2203
Partials 31 31
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
| outerRadius: radius, | ||
| startAngle: -Math.PI / 2 - GaugeComponent.EXTRA_ARC_ANGLE, | ||
| endAngle: -Math.PI / 2 - GaugeComponent.EXTRA_ARC_ANGLE + (inputData.value / inputData.maxValue) * Math.PI | ||
| startAngle: -completeAngle, |
There was a problem hiding this comment.
Does this handle the first case described in the comment? If inputData.value is 0, we're doing [-completeAngle, -completeAngle] so there'd still be no arc
There was a problem hiding this comment.
Yes. We want no value arc, when the inputData.value is 0. In fact @anandtiwary mentioned that's the expected outcome as well
case 1: inputData.value = 0, the end angle would be -Math.PI / 2 - GaugeComponent.EXTRA_ARC_ANGLE which is same as start angle. So the arc will be completely empty. Expected outcome.
There was a problem hiding this comment.
Ah I misread his comment - I thought our ux generally has a little arc for a 0-value gauge to show it's "emptiness"? I'm pretty sure that's what we do in the linear case.
There was a problem hiding this comment.
hmm. fair point. I can use a value of 0.5, if the value is 0 to display the 0-value arc
There was a problem hiding this comment.
I think that makes sense - something like percentFill = Math.max(0.01, inputData.value / inputData.maxValue)
There was a problem hiding this comment.
I am not sure it looks any better to show a little arc for 0.
Also, could we rename completeAngle to just startAngle and assign it -completeAngle ?
There was a problem hiding this comment.
Also, could we rename
completeAngleto just startAngle and assign it-completeAngle
Didn't catch the comment
There was a problem hiding this comment.
CompleteAngle is confusing to me. Can we do const startAngle = - (Math.PI / 2 + GaugeComponent.EXTRA_ARC_ANGLE); ?
There was a problem hiding this comment.
So, endAngle = startAngle + 2 * (inputData.value / inputData.maxValue) * (- startAngle)
This comment has been minimized.
This comment has been minimized.
| outerRadius: radius, | ||
| startAngle: -completeAngle, | ||
| endAngle: -completeAngle + 2 * (inputData.value / inputData.maxValue) * completeAngle | ||
| endAngle: -completeAngle + 2 * Math.max(0.05, inputData.value / inputData.maxValue) * completeAngle |
There was a problem hiding this comment.
.05 feels high here, how does it look? It's showing a 5% fill in the empty case - I was thinking a 1% fill would be sufficient.
There was a problem hiding this comment.
0.05 looks high, as you said. Looks like a real +ve value.
0.02 seems to look good as compared to 0.01
This comment has been minimized.
This comment has been minimized.
|
Anything more needed on this PR? For max risk score, the gauge would not appear full. This seems like an important bug |
yes we should fix. I think the consensus was no nub, right @jake-bassett ? |
got rid of the nib computation |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Fixes #486 (comment)