Skip to content

Moved drawing of radial lines before drawing the tick labels#5855

Merged
simonbrunel merged 1 commit into
chartjs:masterfrom
fhp:master
Nov 27, 2018
Merged

Moved drawing of radial lines before drawing the tick labels#5855
simonbrunel merged 1 commit into
chartjs:masterfrom
fhp:master

Conversation

@fhp

@fhp fhp commented Nov 22, 2018

Copy link
Copy Markdown
Contributor

such that the radial lines are not drawn on top of the tick labels and their backdrop.

…at the radial lines are not drawn on top of the tick labels and their backdrop.
@simonbrunel simonbrunel requested a review from nagix November 25, 2018 16:47
@simonbrunel

Copy link
Copy Markdown
Member

@nagix since you worked on this scale recently, do you think of any issue moving the order of drawing?

@nagix

nagix commented Nov 26, 2018

Copy link
Copy Markdown
Contributor

I don't see any issues. This looks good to me.

@simonbrunel

Copy link
Copy Markdown
Member

@fhp do you have a fiddle that shows the issue this PR is trying to solve (or an associated ticket)?

We are also drawing text in drawPointLabels() so I want to be sure we are not generating another issue moving this call.

@fhp

fhp commented Nov 27, 2018

Copy link
Copy Markdown
Contributor Author

@simonbrunel yes: http://jsfiddle.net/ajbrk59c/

And using this fix: http://jsfiddle.net/7xL3jtzk/ (change is in the used dependency)

I'm not very familiar with the Chart.js codebase, so I am unsure if it introduces other issues.

@nagix

nagix commented Nov 27, 2018

Copy link
Copy Markdown
Contributor

As point labels are drawn outside the outermost circle, I think there is no problem.

@simonbrunel simonbrunel added this to the Version 2.8 milestone Nov 27, 2018
@simonbrunel simonbrunel merged commit 08447e9 into chartjs:master Nov 27, 2018
@simonbrunel

Copy link
Copy Markdown
Member

Thanks @fhp

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