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

add 10 to yLabelWidth on set, rather than on use #677

Merged
merged 1 commit into from
Apr 12, 2015

Conversation

rylanhalteman
Copy link
Contributor

Padding on the line chart's y-axis labels can get crowded because when checking the x-axis label rotation (line 1506) it checks for this.yLabelWidth + 8 rather than this.yLabelWidth + 10.
Ex:
screen shot 2014-10-08 at 11 38 08

Changing this check to yLabelWidth + 10 fixes this issue. However, since the yLabelWidth variable is either used to only check against itself (in the fit() method, lines 1462 & 1468), or checking against yLabelWidth + 10 (line 1487 * 2, line 1506), I moved the addition to the setting of yLabelWidth, rather than when checking the variable.

Result:
screen shot 2014-10-08 at 11 52 47

The result is correctly padded y-axis labels.

@pfirpfel
Copy link

pfirpfel commented Nov 5, 2014

Would solve #729 and #440

@ccsmail
Copy link

ccsmail commented Nov 25, 2014

Any chance this could be merged? Would be really helpful.

@boroth
Copy link

boroth commented Dec 10, 2014

+1

@jakesylvestre
Copy link
Contributor

@rylanhalteman Can you please repoint this PR to the dev branch? Thanks. If your not sure how to repoint this PR please see: http://stackoverflow.com/questions/24159036/how-to-modify-a-pull-request-on-github-to-change-target-branch-to-merge-into

@derekperkins derekperkins modified the milestone: 1.1 Dec 30, 2014
@fulldecent
Copy link
Contributor

I am +1 on this. Any other votes?

@etimberg
Copy link
Member

Looks good to me. I am +1 as well.

etimberg added a commit that referenced this pull request Apr 12, 2015
add 10 to yLabelWidth on set, rather than on use
@etimberg etimberg merged commit 604d6bd into chartjs:master Apr 12, 2015
@wildchild
Copy link

Thanks for fix and merging.

@rylanhalteman
Copy link
Contributor Author

@jakesyl @fulldecent @etimberg My apologies for being unresponsive. I finished my project and was filtering out github emails by mistake. :( Thanks for the merge!

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

Successfully merging this pull request may close these issues.

9 participants