Skip to content

Handle rangebreak gaps in candlestick & ohlc #4814

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

Merged
merged 4 commits into from
May 7, 2020
Merged

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented May 5, 2020

Fixes #4742 and fixes #4795 by hangling gaps in distinctVals function.
Demo1: Before | After
Demo2: Before | After

@plotly/plotly_js

@archmoj archmoj added bug something broken status: reviewable labels May 5, 2020
@archmoj archmoj added this to the v1.54.2 milestone May 5, 2020
@archmoj archmoj requested a review from nicolaskruchten May 6, 2020 14:04
"data": [
{
"type": "candlestick",
"x": [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in this mock, it would be great to see some data in the next visible range, like after 9am the next day, just to capture some of the tick labelling etc

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I added another mock in 8a4bae3.
And we are going to revise tick behaviour (including these mocks) in #4734 after this PR is merged.

@archmoj archmoj requested a review from alexcjohnson May 6, 2020 20:48
var first;
for(first = 0; first < vals.length; first++) {
if(vals[first] !== BADNUM) break;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh interesting, I wouldn't have guessed that the problem traced back to distinctVals, nice find!

TIL:

all undefined elements are sorted to the end of the array, with no call to compareFunction

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort

So perhaps all we need is the loop to find last and the rest of this can stay as it was, as we'll never have undefined anywhere else after a sort?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe worth testing this in IE before relying on it. I've confirmed in Chrome, FF, Safari on my mac so seems robust on modern browsers.

If it were to fail on IE, I wouldn't trust that sorting works at all then with interspersed undefined values, and better would be to filter these out before even starting (instead of the slice() call)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson good catch! Not working on IE11.

Copy link
Contributor Author

@archmoj archmoj May 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson Correction. It appear to be working on IE11.

IE11

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson correction number 2: It is sometimes working and sometime not.
So let's fix that for IE11.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. The logic is modified by 2f3b098 & tested in IE11 and it works.

@archmoj
Copy link
Contributor Author

archmoj commented May 7, 2020

Also we should possibly addess #4818 in this PR which seems related to applying distinctVals.

@archmoj
Copy link
Contributor Author

archmoj commented May 7, 2020

Also we should possibly addess #4818 in this PR which seems related to applying distinctVals.

The heatmap bug is not related to distinctVals at all. So we are not going to fix that in this PR.

@archmoj archmoj requested a review from alexcjohnson May 7, 2020 19:08
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thanks for checking on IE11. 💃

@archmoj archmoj merged commit 2a4f20e into master May 7, 2020
@archmoj archmoj deleted the rangebreaks-ohlc branch May 7, 2020 20:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using hour rangebreaks causes candlestick chart to fail Candlestick body width bug with range breaks
3 participants