Rotations between 180-360 degrees on x axis labels #5246
Conversation
|
Sample image, before and after this change |
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
| @@ -709,6 +709,12 @@ module.exports = Element.extend({ | |||
| var xTickEnd = options.position === 'right' ? me.left + tl : me.right; | |||
| var yTickStart = options.position === 'bottom' ? me.top + axisWidth : me.bottom - tl - axisWidth; | |||
| 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 |
Text rotated -90° does not work correctly, example at this fiddle : https://jsfiddle.net/thLyc5wh/7/
Changes are