use arc calculations to render range values #86
Conversation
rkyle35242
requested review from
jkhusanov,
calebmackdavenport,
chaceburnette,
joshuakg,
rykugur,
kathrynnlester and
taylorcore
as code owners
| if (previousArc) end = previousArc.end + spaceBetweenSegments; | ||
|
|
||
| let start = end; | ||
| if (arc) start = arc.end; |
There was a problem hiding this comment.
This was very confusing until I realized the start and end are kind of reversed. I believe it's due to the textAnchor enum on the RNSVG Text component.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Is there ever a case where someone wouldn't want these to line up with the segments? I am not sure this should be a flag, rather should just be the default behavior to always be aligned with the segments.
I tested out your branch and am getting issues with the first label when the arc is over 180 degrees.
src/components/RangesDisplay.js
Outdated
| const arc = arcs[index]; | ||
| const previousArc = arcs[index - 1]; | ||
|
|
||
| let end = 0; |
There was a problem hiding this comment.
| let end = 0; | |
| let end = arcsStart + index * (arcSegmentDegree + spaceBetweenSegments); |
There was a problem hiding this comment.
The end is already calculated in the SegmentedArc component, and duplicating the calculation here might result in deviations if we change it. The initial end should just be the start of the arc rather than 0, as arcs > 180 degrees have a negative degree starting point. Now I'm realizing this was probably an issue on arcs narrower than 180 degrees as well
|
@dakota-kallas while I don't know that there is a use case to have the ranges not aligned with scaled segments, I think it's possible that someone would want to. I'm in favor of more control over less here. That being said, I'll look into the > 180 degrees label issue you noted and update the "alignRangesWithSegments" to default to |
| @@ -27,6 +27,7 @@ export const SegmentedArc = ({ | |||
| rangesTextStyle = styles.rangeTextStyle, | |||
| capInnerColor = '#28E037', | |||
| capOuterColor = '#FFFFFF', | |||
| alignRangesWithSegments = true, | |||
There was a problem hiding this comment.
true as the default is a good idea. It was a bit confusing before why we would want to default to false.
There was a problem hiding this comment.
I had a 50/50 shot of getting it right 😉
Issue reported here: #84.
Ranges are placed evenly across the arc. However, there could be cases where the user wants the ranges to align with the segments. So I'm adding a flag to align the ranges display with the segments.