Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

EventHandler produces reversed TimeRanges #454

Open
brandly opened this issue Sep 17, 2020 · 2 comments
Open

EventHandler produces reversed TimeRanges #454

brandly opened this issue Sep 17, 2020 · 2 comments

Comments

@brandly
Copy link
Contributor

brandly commented Sep 17, 2020

🐛Bug report

Before calling onZoom, EventHandler creates the necessary TimeRange that will be passed to onZoom:

const newTimeRange = new TimeRange([newBegin, newEnd].sort());
if (this.props.onZoom) {
this.props.onZoom(newTimeRange);
}

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:

  1. View a ChartContainer with enableDragZoom set to true
  2. Drag to select a time range before 1970
  3. View the time range passed to your onTimeRangeChanged handler

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.

@brandly
Copy link
Contributor Author

brandly commented Sep 17, 2020

i can send a PR. some options:

  1. do this inline
const newTimeRange = new TimeRange(newBegin < newEnd ? [newBegin, newEnd] : [newEnd, newBegin]); 
  1. pull that into a function
const ascendingDates = (a, b) =>
  a < b ? [a, b] : [b, a]

// ...

const newTimeRange = new TimeRange(ascendingDates(newBegin, newEnd)); 
  1. 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?

@pjm17971
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants