Skip to content
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

Merged
merged 1 commit into from
Nov 2, 2018

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Jun 29, 2018

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
screen shot 2018-06-30 at 5 18 34 amscreen shot 2018-06-30 at 4 59 50 am

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
screen shot 2018-06-30 at 5 17 42 amscreen shot 2018-06-30 at 4 57 58 am

@benmccann
Copy link
Contributor

This looks like a good improvement to me! Would it be possible to write a test case for this?

@nagix
Copy link
Contributor Author

nagix commented Jul 7, 2018

core.scale tests have been added.

@nagix nagix force-pushed the issue-5609 branch 3 times, most recently from 0c51aec to d238044 Compare July 7, 2018 18:56
etimberg
etimberg previously approved these changes Jul 9, 2018
@@ -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() ?
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

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;
}

Copy link
Contributor Author

@nagix nagix Aug 9, 2018

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.

Copy link
Contributor

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

Copy link
Contributor

@benmccann benmccann left a 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

benmccann
benmccann previously approved these changes Aug 9, 2018
@simonbrunel
Copy link
Member

@nagix we are considering a new patch version (2.7.3) so I wondering if we can merge this PR without #4658? or should we wait for both and release in 2.8?

@nagix
Copy link
Contributor Author

nagix commented Sep 16, 2018

There is no problem with merging this PR without #4658 as it can be handled separately.

simonbrunel
simonbrunel previously approved these changes Nov 1, 2018
Copy link
Member

@simonbrunel simonbrunel left a 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?

@nagix
Copy link
Contributor Author

nagix commented Nov 2, 2018

I have rebased and resolved the conflicts.

@simonbrunel simonbrunel merged commit f74699a into chartjs:master Nov 2, 2018
@simonbrunel
Copy link
Member

Thanks @nagix

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants