Rotations between 180-360 degrees on x axis labels#5246
Rotations between 180-360 degrees on x axis labels#5246teroman wants to merge 6 commits intochartjs:masterfrom
Conversation
|
Sample image, before and after this change |
etimberg
left a comment
etimberg
left a comment
There was a problem hiding this comment.
Left a comment on the code. I would say that a test needs to be written for this otherwise it's pretty easy for it to accidentally get removed later
src/core/core.scale.js
Outdated
| var yRotationOffset = 0; | ||
|
|
||
| if (me.labelRotation < 0) { | ||
| var longestText = helpers.longestText(context, tickFont.font, labelsFromTicks(ticks), me.longestTextCache); |
There was a problem hiding this comment.
I think you can avoid looking this up by using me.longestLabelWidth
|
Updated the longestLabelWidth as you recommended, but I'm not sure were to go to write the test. Is there a similar example I could take inspiration from? |
|
In terms of a test, I think this is a good test to use #3988 @simonbrunel thoughts? |
|
Unfortunately, image based tests don't work when there is text because font rendering is not stable between browsers. |
Text rotated -90° does not work correctly, example at this fiddle : https://jsfiddle.net/thLyc5wh/1/ Changes are 1) Take abs() of sin/cos when measuring text so that sizes are always positive. Text was measured as having a negative width. 2) Move label drawing offset by the text length so that it is drawn outside the chart area, below the axis.
white space fixes
white space
|
@etimberg @simonbrunel do you still want a test written for this one? If so, do you have any advice on how it could be tested? |
benmccann
changed the title
| var yTickEnd = options.position === 'bottom' ? me.top + axisWidth + tl : me.bottom + axisWidth; | ||
| var yRotationOffset = 0; | ||
|
|
||
| if (me.labelRotation < 0) { |
There was a problem hiding this comment.
This doesn't seem right. What if the labelRotation is 270 degrees? We should make sure it works in that case as well. I think what you want is to check if (Math.sin(labelRotationRadians) < 0) ?
|
@teroman would you be able to update this to address my comment? |
|
@teroman would you be able to give this PR another look? |
|
@teroman would you be able to update this PR to address the comment? |
|
@teroman we'd love to get this PR in. Just a quick reminder that there's a pending comment |
|
@teroman are you giving up on this PR? I'd love to get it in if you can quickly respond to the issue I raised |
|
I'm closing this PR given the lack of response. Please feel free to reopen if you're able to update it |
4 participants
Text rotated -90° does not work correctly, example at this fiddle : https://jsfiddle.net/thLyc5wh/7/
Changes are