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

Don't draw tick across axis/border #5178

Merged
merged 2 commits into from Jan 28, 2018
Merged

Conversation

jhaenchen
Copy link
Contributor

@jhaenchen jhaenchen commented Jan 23, 2018

In the current version of chartjs, drawing ticks overlaps the tick line with the border or axis line, making those intersections darker. This PR takes that into account when drawing ticks or borders and offsets them appropriately.

Before:
https://codepen.io/anon/pen/dJLaje
image

I know it's small but you can probably see the darker pixel where the tick overlaps with the border.

After:
image

As you can see, no dark pixel!

In the future, I suggest spending even more time keeping lines from overlapping, even when gridlines intersect. This work is a small step in that direction.

@jhaenchen
Copy link
Contributor Author

I think it would also be worthwhile to make the tick axis offset a configurable value, similar to label padding.

simonbrunel
simonbrunel previously approved these changes Jan 24, 2018
@jhaenchen
Copy link
Contributor Author

Don't merge this. If the x axis is not shown and showBorder on the x axis is also true, the y axis will not extend to the bottom of the first tick. I'm not sure how to solve this problem since one axis needs to be responsible for that pixel, so hiding the responsible will always make the other look short.

@jhaenchen
Copy link
Contributor Author

image

@jhaenchen
Copy link
Contributor Author

I'm thinking of drawing that single intersection pixel separately if either axis is defined. Not my favorite solution...

@etimberg
Copy link
Member

Sounds like https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L911 could be wrong, or the tick is off by 1 pixel

@jhaenchen
Copy link
Contributor Author

@etimberg Part of my change is that ticks never contribute to any of the border or axis, they're always offset from the border. The issue is that when drawing each border, I don't know if the other border has already taken care of the pixel at the intersection of the two borders, so that pixel gets drawn twice. In this PR, currently, the horizontal border draws that intersection pixel, which is why hiding the horizontal border removes that last pixel.

@etimberg
Copy link
Member

Right, hmm. There was a PR a while ago that moved into an internal plugin but it didn't get finished (#4117). Perhaps that would be better solved then

@jhaenchen
Copy link
Contributor Author

An easy compromise would be to just allow this one pixel to be darker. It still solves the problem of the ticks making the borders darker.

@etimberg
Copy link
Member

I am ok with that compromise

@jhaenchen
Copy link
Contributor Author

Okay, all done.

@simonbrunel
Copy link
Member

@jhaenchen is this PR ready to be merged?

@jhaenchen
Copy link
Contributor Author

@simonbrunel yes.

@simonbrunel simonbrunel merged commit e61392a into chartjs:master Jan 28, 2018
@simonbrunel
Copy link
Member

Thanks!

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.

None yet

3 participants