-
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
fix #4216 when dataset empty, set scale min max to default if null #4258
Conversation
Have no idea why
|
src/scales/scale.linear.js
Outdated
me.min = isFinite(me.min) ? me.min : DEFAULT_MIN; | ||
me.max = isFinite(me.max) ? me.max : DEFAULT_MAX; | ||
me.min = (me.min && isFinite(me.min)) ? me.min : DEFAULT_MIN; | ||
me.max = (me.max && isFinite(me.max)) ? me.max : DEFAULT_MAX; |
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.
Is it intentional to use DEFAULT_MAX
if max === 0
(same for min)?
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.
Sorry, meant to be not null. Have modified the commit.
a69c2b4
to
7d89160
Compare
src/scales/scale.linearbase.js
Outdated
@@ -47,10 +47,10 @@ module.exports = function(Chart) { | |||
} | |||
} | |||
|
|||
if (me.min === me.max) { |
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.
The previous commit is not enough. Say when we set min: 10
(whatever larger than default max), will result in me.min > me.max here but not processed correctly.
I changed this to be based on the opts' min setting if exists.
Just checked the two new failed tests, I guess they are set wrong. When I change the DEFAULT_MIN to be -1, the two tests will pass, but another test will fail. Should I create another test here for the case when tick.min is set, max value should be larger than the config min value? |
Looks good to me. Just needs a rebase and we can merge |
5d5e67f
to
6d1bb3c
Compare
4d20b89
to
907ee6c
Compare
squashed into 1 commit |
Looks like this got fixed in #4425 |
Closing per @benmccann's comment |
Currently, when dataset empty, we do not check whether min, max is null.
https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.linear.js#L102-L128
Set it to default if null could prevent axis's ticks array being [0, NaN], which led to the issue #4216