-
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 offsetGridLine behavior with a single data point #5609
Conversation
This looks like a good improvement to me! Would it be possible to write a test case for this? |
core.scale tests have been added. |
0c51aec
to
d238044
Compare
@@ -80,7 +80,11 @@ function getLineValue(scale, index, offsetGridLines) { | |||
var lineValue = scale.getPixelForTick(index); | |||
|
|||
if (offsetGridLines) { | |||
if (index === 0) { | |||
if (scale.getTicks().length === 1) { | |||
lineValue -= scale.isHorizontal() ? |
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.
I think this draws a line on top of the axis? The axis looks extra dark in your images, which further reinforces my reading of this code. Perhaps it should return null instead and then not draw the line in that case
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.
Yes, it draws a line on top of the axis, but this is not a special case as you can see in this official example (the y axis looks extra dark depending on the canvas width). This can be fixed by checking if the line and the axis border are overlapping, but wouldn't it be better to open a different PR?
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.
It'd be an easy change:
var xLineValue = getPixelForGridLine(me, index, gridLines.offsetGridLines);
if (helpers.isNullOrUndef(xLineValue)) {
return;
}
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.
This is a bit more complicated. If it returns here, not only the grid line but also the tick label will disappear as they are drawn together. That's why lineColor
is set to transparent if the offset grid line is out of the range.
So, in order to avoid drawing the grid line over axis, we need to check whether each grid line is on the axis (this requires to check gridLines.drawBorder
and options.position
) and to set lineColor
to transparent if needed.
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.
Ah, right you are. Ok, in that case, I agree with you that it's complicated enough that it would perhaps be better handled in another PR instead. Thanks for the clarification
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.
Avoid drawing line over axis
There is no problem with merging this PR without #4658 as it can be handled separately. |
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.
@nagix is this PR still valid? if so, can you rebase then I will merge it?
I have rebased and resolved the conflicts. |
Thanks @nagix |
When a chart has only one data point,
offsetGridLine: true
does nothing. As you can see in the examples below, a grid line goes in the middle of the bar or point (1, 3, 5 and 7). These are inconsistent behaviours with other cases (2, 4, 6 and 8).Current version: https://jsfiddle.net/nagix/9b6h3L2x
This PR fixes the issue so that
offsetGridLine
words for a single data point (1, 3, 5 and 7).Proposed version (+ #4658): https://jsfiddle.net/nagix/wtpqLn5a