-
Notifications
You must be signed in to change notification settings - Fork 12k
Fixes Label Array Tick alignment and Centering #5248
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
Conversation
src/core/core.scale.js
Outdated
|
|
||
| var label = itemToDraw.label; | ||
| if (helpers.isArray(label)) { | ||
| if (label.length % 2 === 0) { |
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.
what's the reason for this if? If I understand correctly, -tickFont.size * label.length 2 is half the height of the label so we should just move up by that amount. I think this is working around simply setting a different text baseline that would make the code cleaner
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.
The reason for that if is to check if there is an even amount of labels. If so then the labels get centered evenly since it would be a different calculation when compared to an odd amount of labels. I think you are meaning -tickFont.size * label.length / 2? I'm fairly new to the project but I think setting a different baseline could work as well like you said. Thank you for the review.
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.
simonbrunel
left a comment
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.
How these changes behave with multi-lines labels on an horizontal scale? Can you upload your changes in a fiddle?
src/core/core.scale.js
Outdated
|
|
||
| var label = itemToDraw.label; | ||
| if (helpers.isArray(label)) { | ||
| if (label.length % 2 === 0) { |
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.
src/core/core.scale.js
Outdated
| context.translate(0, (-tickFont.size * (label.length / 2)) / 2); | ||
| } else { | ||
| context.translate(0, -tickFont.size * (label.length / 2)); | ||
| } |
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.
We need to use the line height (not only the font size) and it would be easier and faster to translate y instead of translating the context?
var lineCount = label.length;
var lineHeight = tickFont.size * 1.5;
var y = - lineHeight * lineCount / 2;
var i = 0;
for (; i < lineCount; ++i) {
context.fillText('' + label[i], 0, y);
y += lineHeight;
}
|
Good idea with the lines @simonbrunel, I thought they were even, my bad. I took your suggestions and made some changes. Here are the two fiddles that address both scenarios Thanks again for feedback and review. |
src/core/core.scale.js
Outdated
| for (var i = 0, y = 0; i < label.length; ++i) { | ||
| var lineCount = label.length / 2; | ||
| var lineHeight = tickFont.size * 1.5; | ||
| var y = 0; |
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.
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.
When I do:
var lineCount = label.length / 2;
var lineHeight = tickFont.size * 1.5;
var y = me.isHorizontal() ? 0 : -lineHeight * lineCount;
Which seems just slightly off. I think the equation is missing something small or I'm doing it wrong.
|
I think this resolves the slight offset |
src/core/core.scale.js
Outdated
| var label = itemToDraw.label; | ||
| if (helpers.isArray(label)) { | ||
| for (var i = 0, y = 0; i < label.length; ++i) { | ||
| var lineCount = label.length / 2; |
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.
Should be var lineCount = label.length; and re-used in the for loop.
src/core/core.scale.js
Outdated
| for (var i = 0, y = 0; i < label.length; ++i) { | ||
| var lineCount = label.length / 2; | ||
| var lineHeight = tickFont.size * 1.5; | ||
| var y = me.isHorizontal() ? 0 : -lineHeight * lineCount / 1.2; |
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 the correct calculation depends on the actual text baseline:
a. if textBaseline: 'top' then y = - lineHeight * lineCount / 2;
b. if textBaseline: 'middle' then y = - lineHeight * (lineCount - 1) / 2;
c. if textBaseline: 'bottom' then y = - lineHeight * (lineCount - 2) / 2;
By default, textBaseline === 'middle' (line 738), so b. should work in our case?
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.
Oh ok makes sense, b does work in this case. Is it necessary to adjust the code for each textBaseline value?
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.
Since it's always middle for vertical scale, I would only implement b. for now.
src/core/core.scale.js
Outdated
| var lineHeight = tickFont.size * 1.5; | ||
| var y = me.isHorizontal() ? 0 : -lineHeight * lineCount / 1.2; | ||
|
|
||
| for (var i = 0; i < label.length; ++i) { |
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.
We should use lineCount instead of label.length
|
Thanks @MPierre9 |
|
Thanks for all the help @simonbrunel and @etimberg |
|
Thanks for the fix guys, I really appreciate it. Finally have time to try it out today :) |


Fixes #5211 by centering the array tick labels depending on if they are even or odd.
Before
Example JSFiddle curtosy of @Moghul
After
Note: I show an even number of labels on the second bar to show how the code adjusts for this