Fixed HorizontalBar: stacked axis, displays NaN when all legends #3792
Conversation
src/scales/scale.linear.js
Outdated
| * @param Integer {max} chart maximum | ||
| * @return Nothing - no return value | ||
| * */ | ||
| takeScaleLimitSnapShot: function(min, max) { |
There was a problem hiding this comment.
I'd prefer this to be a private function that takes the scale as the first parameter. Since it's not needed elsewhere
There was a problem hiding this comment.
Agree, outside the scale prototype: function takeScaleLimitSnapShot(min, max) { ... Better API and minification!
src/scales/scale.linear.js
Outdated
| } | ||
|
|
||
| // Take a snapshot of scale limit to be used if invalid values for { max, min } | ||
| me.takeScaleLimitSnapShot(me.min, me.max); |
There was a problem hiding this comment.
Will this work if the chart is created with all datasets hidden?
ie, adding hidden: true to each dataset on chart creation
There was a problem hiding this comment.
It should, i've added the test case for that. This will test should cover that right ?
src/scales/scale.linear.js
Outdated
| /** | ||
| * A util function to create snapshot of axis limits {max, min} | ||
| * @Method chart.takeScaleLimitSnapshot | ||
| * @param Integer {min} chart mininmum |
There was a problem hiding this comment.
The JSDoc comments aren't quite correct syntax wise. The data type is inside {}. Further, these aren't just integers since the scale could use floating point.
`* param {Number} min - chart minimum
There was a problem hiding this comment.
ah you're right. i'll update that
There was a problem hiding this comment.
And you can also remove the @method which is useless (and actually wrong :) )
BTW, I'm going to setup jdoc as a dev dependency and add a gulp task to generate it (will do a pass on all the existing code comments to fix them).
src/scales/scale.linear.js
Outdated
|
|
||
| var LinearScale = Chart.LinearScaleBase.extend({ | ||
|
|
||
| scaleLimitSnapshot: { |
There was a problem hiding this comment.
Since this is only used once, I think it could be returned from the takeScaleLimit function rather than storing it on the axis
There was a problem hiding this comment.
agreed! i'll make those changes
|
my code failing due to some test cases. An example of one of the test: https://github.com/chartjs/Chart.js/blob/master/test/scale.linear.tests.js#L404-L427 I am not sure these test cases do what they describe, for example: 'Should ensure that the scale has a max and min that are not equal when beginAtZero is set'
expect(chart.scales.yScale0).not.toEqual(undefined); // must construct
expect(chart.scales.yScale0.min).toBe(0);
expect(chart.scales.yScale0.max).toBe(1);Am I missing something ? |
.gitignore
Outdated
|
|
||
| *.swp | ||
| **/npm-debug.log | ||
| samples/test |
There was a problem hiding this comment.
Can you remove the samples/test ignore since it's quite a personal setting?
There was a problem hiding this comment.
sure, i'll do that.
src/scales/scale.linear.js
Outdated
| /** | ||
| * A util function to create snapshot of axis limits {max, min} | ||
| * @Method chart.takeScaleLimitSnapshot | ||
| * @param Integer {min} chart mininmum |
There was a problem hiding this comment.
And you can also remove the @method which is useless (and actually wrong :) )
BTW, I'm going to setup jdoc as a dev dependency and add a gulp task to generate it (will do a pass on all the existing code comments to fix them).
src/scales/scale.linear.js
Outdated
| * @param Integer {max} chart maximum | ||
| * @return Nothing - no return value | ||
| * */ | ||
| takeScaleLimitSnapShot: function(min, max) { |
There was a problem hiding this comment.
Agree, outside the scale prototype: function takeScaleLimitSnapShot(min, max) { ... Better API and minification!
|
Those tests ensure that when there is no data, the axis range gets set to something. The default range is [0, 1]. Could we do the same thing in this case? |
|
@etimberg, @simonbrunel thanks for the concise review guys! after clarification about the test cases, I am able to come up with a really simple solution! I'll add PR in a bit |
Jareechang
force-pushed
the
issue/#3770
branch
2 times, most recently
from
42cfb07 to
82d9678
Compare
unselected (chartjs#3770) added ability to take snapshot of chart limits in order to be used when max and min value is not finite added ignore files
|
@etimberg and @simonbrunel, please review when you guys get a chance. thanks! ✨ |
3 participants
please review when you get a chance!