EventHandler produces reversed TimeRanges
🐛Bug report
Before calling onZoom, EventHandler creates the necessary TimeRange that will be passed to onZoom:
https://github.com/esnet/react-timeseries-charts/blob/aa9c9b368100d78337b562d9e2833f2d90d9de3d/src/components/EventHandler.js#L206-L209
The desired behavior is to produce a TimeRange where timeRange.begin() < timeRange.end(). However, array sort in JS is... interesting. To quote MDN:
all non-undefined array elements are sorted by converting them to strings and comparing strings in UTF-16 code units order.
For dates before 1970, newBegin and newEnd will be negative numbers. In many cases, these numbers will be consistently reversed. For example, in the node repl:
$ node
Welcome to Node.js v14.4.0.
Type ".help" for more information.
> [-390040203141, -204123317277].sort()
[ -204123317277, -390040203141 ]
To Reproduce Steps to reproduce the behavior:
- View a
ChartContainerwithenableDragZoomset totrue - Drag to select a time range before 1970
- View the time range passed to your
onTimeRangeChangedhandler
Expected behavior
The values should be properly sorted. onZoom is called in three places. Before the other two calls, no sorting takes place.
The other two places might be relying on the surrounding context for sort order, but I think it'd be best to explicitly sort in all three spots.
i can send a PR. some options:
- do this inline
const newTimeRange = new TimeRange(newBegin < newEnd ? [newBegin, newEnd] : [newEnd, newBegin]);
- pull that into a function
const ascendingDates = (a, b) =>
a < b ? [a, b] : [b, a]
// ...
const newTimeRange = new TimeRange(ascendingDates(newBegin, newEnd));
- sort any array of numbers in place
const sortNumbers = (numbers) =>
numbers.sort((a, b) => {
if (a < b) return -1
if (b > a) return 1
return 0
})
// ...
const newTimeRange = new TimeRange(sortNumbers([newBegin, newEnd]));
@pjm17971 what do you think?
I guess this came from a PR ages ago.
It's kind of too bad TimeRange doesn't just suck it up and order them if they are out of order. But to fix it here I guess the first approach is fine.