TimeSeriesChart: Fix horizontal line rendering.#576
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| lines.forEach((line: any) => { | ||
| const points: any[] = line.props?.points ?? []; | ||
| if (points.length > 1 && points.every((point: any) => point.y === points[0].y)) { | ||
| points[0].y += 0.001; |
There was a problem hiding this comment.
For clarity: this adjustment is smaller than any precision we ever show in a tooltip or anything, right?
I guess my two dumb questions are this:
- What's the smallest amount it can be adjusted and have this work? Is it somehow relative to the y height of the chart (because presumably that's going to factor into whether the line is technically on the same horizontal pixel line?
- Is there any reason not to just do this always? i.e. jiggle this point without first checking that all of the values are identical? I realize that check is probably not that bad 99.9% of the time, but in the rare cases like intraday heart rate that there's a lot more points than even 6 months of data... it could matter.
There was a problem hiding this comment.
For clarity: this adjustment is smaller than any precision we ever show in a tooltip or anything, right?
With this fix, we aren't adjusting the values at all, just the y position of the first point in the line's path, so it won't affect the display of any of the actual values in tooltips or ticks or anything like that.
- What's the smallest amount it can be adjusted and have this work? Is it somehow relative to the y height of the chart (because presumably that's going to factor into whether the line is technically on the same horizontal pixel line?
It doesn't seem to be affected by the chart height at all. It is just an artifact of how the browser handles SVG path strokes that are gradients. So all that needs to be done is tricking the browser into thinking the line path has some height to it.
The smallest value that worked for me was 0.0005. Anything less failed to draw the line. It probably gets rounded to 0.001 because 0.00049 doesn't work. Rather than going with 0.0005 and being right on the bleeding edge, I went with 0.001 in case there is any variance in the rounding by different browsers (I tested with Chrome and FireFox).
- Is there any reason not to just do this always? i.e. jiggle this point without first checking that all of the values are identical? I realize that check is probably not that bad 99.9% of the time, but in the rare cases like intraday heart rate that there's a lot more points than even 6 months of data... it could matter.
I think we probably could if we wanted to. I suspect that the most common scenario for horizontal lines is probably the survey answer chart, which usually won't have too many points (unlike the intraday HR you mentioned). It seems unlikely that device data would be identical across the entire line, but I suppose it could very early on in the data collection.
That said, as long as the data has any variance at all (which is likely with the intraday HR data), the every call here will short circuit on the first non-match. So as long as the data is variable, it should only need to check a few points in order to determine that the jitter is not needed. In cases where the jitter is needed, it would loop the entire list of points, but I think that may be fairly infrequent.
|
Thanks @ajmenca. |
2 participants
Overview
Fixes: #535
This branch adds a fix for rendering horizontal lines in
TimeSeriesChart. Rather than jittering the data, this fix jitters the first point'syposition very slightly to cause the bounding box to have some height. This causes the gradient stroke to render.Security
No new security risk. Just a rendering tweak to better handle an edge case.
Testing
I've added a new story to the
TimeSeriesChartstorybook.Documentation
@CareEvolution/api-docs.