-
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 issue #4441 - y-axis labels partially hidden due to restrictive initial fitting. #4942
Conversation
- y-axis labels partially hidden due to restrictive initial fitting.
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 is a safe change to make. The reason it was initialized to 50% height and width was to try and ensure that there was always area to show the chart on. Are the chartAreaHeight
and chartAreaWidth
variables still used? If not, we should remove them too.
I don't know that this will fix all the issues though. I think it's still possible to get the same case even when we start with the full area. A more generic solution would be to run fitting inside a loop until the sizes are stable but that has many risks including:
- not guaranteed to converge
- slower
The variable The bug is caused by initially fitting the ticks |
I'm fine with that fix, can we add regression tests in |
I could add a test case to scale.linear.tests.js. |
Seems to make more sense in |
Not so sure it affects all scales. It is the linear spacing spacing that is causing the problem. Logarithmic scale labels always have the same format. |
What I mean is that the code you fixed is ran against all scales (actually layout boxes) and so the associated unit test should be in |
That I understand, the whole point of the PR is that it doesn't change anything for the end result in normal operation. |
@simonbrunel What kind of test would you like to be performed in |
@jcopperfield would it be possible to create a test that initializes the canvas similar to the chart at the very top right. Then the test could assert that the y axis is wide enough to show the labels. Edit: realized my suggestion was what you already implemented. Thoughts @simonbrunel @benmccann ? |
I would simply move the test you already wrote in the layout suite. |
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.
Thanks @jcopperfield
…ctive initial fitting. (chartjs#4942) * Fix issue 4441: - y-axis labels partially hidden due to restrictive initial fitting. * Add regression test to linear scale * Moved regression test from linear scale to core layout service
…ctive initial fitting. (chartjs#4942) * Fix issue 4441: - y-axis labels partially hidden due to restrictive initial fitting. * Add regression test to linear scale * Moved regression test from linear scale to core layout service
Fixes #4441 - y-axis labels partially hidden due to restrictive initial fitting.
The problem is caused by the layout service Step 4 being initially trying a chart area height of 50% of the canvas height. When in the initial case there's only enough space for integer ticks, then the y-axis will reserves only the width for integer tick labels. Later in Steps 5 & 6 this reserved space isn't allowed to increase, even when the allowed height is increased in this step.
A simple solution would be to increase the allowed height in the initial computation step to be
maxChartAreaHeight
instead ofchartAreaHeight
.Issue v2.7
![issue_y-axis_labels_partially_hidden](https://user-images.githubusercontent.com/33193571/32741303-17137d64-c8a6-11e7-82ce-c7bb428cdbf1.png)
![fix-issue_y-axis_labels_partially_hidden](https://user-images.githubusercontent.com/33193571/32741310-1c415cde-c8a6-11e7-895a-76f7a8f3c79c.png)
Proposed fix