-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Skip Infinity datapoints in getRightValue #3133
Conversation
+1 to merge |
@bcongdon It would be good to write a test for this case as well |
I added |
Looks good. Thanks @bcongdon The CI error looks like its from Coveralls again, so not to worry about it :) |
@@ -392,7 +392,7 @@ module.exports = function(Chart) { | |||
// Get the correct value. NaN bad inputs, If the value type is object get the x or y based on whether we are horizontal or not | |||
getRightValue: function(rawValue) { | |||
// Null and undefined values first | |||
if (rawValue === null || typeof(rawValue) === 'undefined') { | |||
if (rawValue === null || typeof(rawValue) === 'undefined' || rawValue === Infinity) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, we would have to test -Infinity
as well.
I would use the isFinite
global function [1] (/!\ not the Number.isFinite
function, which IE doesn't support [2]). The check would need to be after the "object" checks, though.
[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/isFinite
[2] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isFinite
@MatthieuRivaud: Switched the logic to use isFinite() |
@simonbrunel @zachpanz88 can we merge this? |
Looks good to me, merge conflicts should get fixed first |
// isNaN(object) returns true, so make sure NaN is checking for a number | ||
if (typeof(rawValue) === 'number' && isNaN(rawValue)) { | ||
// isNaN(object) returns true, so make sure NaN is checking for a number; Discard Infinite values | ||
if (typeof(rawValue) === 'number' && (isNaN(rawValue) || !isFinite(rawValue))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't isFinite
include !isNaN
?
According MDN:
Returns false if the argument is positive or negative Infinity or NaN; otherwise, true.
So could be simply if (typeof(rawValue) === 'number' && !isFinite(rawValue)) {
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True. I'll make the change.
Merged in 62ef40e |
Addresses #3125.
Skips
Infinity
datapoints ingetRightValue
to fix graph creation glitches resulting from 'infinite' datapoints.