A better fix for stacked bar charts with log axes #4010
Conversation
|
In terms of the axes I think something about the range changed and as a result, the axis has a different max and min value. It fixes both because |
|
Shouldn't this issue be fixed on the logarithmic scale side instead? Controllers should not be aware of the scale internal logic so it's a bit weird to try to "prevent" the scale to fail in the bar controller with some special cases (how it's handled by other chart types?). Maybe the best way to fix that issue is to consider any value "valid" from the controller perspective and handle special cases (such as 0 for log) in scales? |
|
I think that is the correct way to deal with this so I will close this PR and find a solution that works without changing the bar controller. |
|
We can keep working on that PR though :) Also, I think the controller need to be fixed because of the |
|
Yup, I realized that after closing :P Will look into the changes now :) |
…e axis has a user defined minimum value.
etimberg
force-pushed
the
fix/stacked-logarithmic-part2
branch
from
69b763a
to
807b723
Compare
…n the bottom point
|
The 2 commits that I pushed fix the issue without breaking the stacked linear axes and without adding log specific code to the bar controller. In terms of how 0s are handled in the log axis, I think we should brainstorm on what the best solution is. Right now we have https://github.com/chartjs/Chart.js/blob/master/src/scales/scale.logarithmic.js#L201-L219 which adds a '0' tick mark below the minimum value but maybe we need to consider something more general |
| @@ -726,7 +726,7 @@ describe('Logarithmic Scale tests', function() { | |||
| expect(yScale.getPixelForValue(80, 0, 0)).toBeCloseToPixel(32); // top + paddingTop | |||
| expect(yScale.getPixelForValue(1, 0, 0)).toBeCloseToPixel(484); // bottom - paddingBottom | |||
| expect(yScale.getPixelForValue(10, 0, 0)).toBeCloseToPixel(246); // halfway | |||
| expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(32); // 0 is invalid. force it on top | |||
| expect(yScale.getPixelForValue(0, 0, 0)).toBeCloseToPixel(484); // 0 is invalid. force it on bottom | |||
There was a problem hiding this comment.
I think the old behaviour (0 at the top) was wrong for log Y axes
| @@ -112,10 +112,9 @@ module.exports = function(Chart) { | |||
| var me = this; | |||
| var meta = me.getMeta(); | |||
| var yScale = me.getScaleForId(meta.yAxisID); | |||
| var base = yScale.getBaseValue(); | |||
| var original = base; | |||
| var base = 0; | |||
There was a problem hiding this comment.
Not sure but I would tend to think that base still need to be initialized to the scale base value because when not stacked this method returns getBasePixel() which is getPixelForValue(getBaseValue()).
There was a problem hiding this comment.
You're right, I will fix that. Good catch!
There was a problem hiding this comment.
I'm refactoring the bar controller and after a few tests, var base = 0; may be the correct way to calculate the bar positions! You can try it by setting a min on the scale. But that also means return scale.getBasePixel() is wrong.
Seems 2.5 is totally broken when stacked bar and a scale min value defined.
There was a problem hiding this comment.
getBasePixel() will always clip to the screen, so if the min is 50 and the max is 100, it will return 50 for the base pixel even though the bar actually starts as 0. Before we had clipping implemented this was needed because otherwise the bar drew over the axis.
* Undo fix for chartjs#3585 since it has broken the stacked bar charts when the axis has a user defined minimum value. * When a value of 0 is requested for a vertical logarithmic axis, return the bottom point
* Undo fix for chartjs#3585 since it has broken the stacked bar charts when the axis has a user defined minimum value. * When a value of 0 is requested for a vertical logarithmic axis, return the bottom point
Still resolves the original issue, #3585 while also fixing the issue identified for normal stacked charts when the Y axis has a user defined minimum value