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

Fixes Label Array Tick alignment and Centering #5248

Merged
merged 4 commits into from
Feb 9, 2018

Conversation

MPierre9
Copy link
Contributor

@MPierre9 MPierre9 commented Feb 7, 2018

Fixes #5211 by centering the array tick labels depending on if they are even or odd.

Before

Example JSFiddle curtosy of @Moghul

before

After

chart js result

Note: I show an even number of labels on the second bar to show how the code adjusts for this

@@ -857,6 +857,11 @@ module.exports = function(Chart) {

var label = itemToDraw.label;
if (helpers.isArray(label)) {
if (label.length % 2 === 0) {
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand why we need to handle odd/even amount of lines differently and based on your screenshot, it doesn't seem correct when there is 2 lines (I would expect the text centered on the bar):

image

Copy link
Member

@simonbrunel simonbrunel left a 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?

@@ -857,6 +857,11 @@ module.exports = function(Chart) {

var label = itemToDraw.label;
if (helpers.isArray(label)) {
if (label.length % 2 === 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also don't understand why we need to handle odd/even amount of lines differently and based on your screenshot, it doesn't seem correct when there is 2 lines (I would expect the text centered on the bar):

image

context.translate(0, (-tickFont.size * (label.length / 2)) / 2);
} else {
context.translate(0, -tickFont.size * (label.length / 2));
}
Copy link
Member

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;
}

@MPierre9
Copy link
Contributor Author

MPierre9 commented Feb 8, 2018

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

JSFiddle (horizontalBar)

JSFiddle (bar)

Thanks again for feedback and review.

for (var i = 0, y = 0; i < label.length; ++i) {
var lineCount = label.length / 2;
var lineHeight = tickFont.size * 1.5;
var y = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More concise notation:

var y = me.isHorizontal() ? 0 : -lineHeight * lineCount;

But also, I think you need lineHeight instead of tickFont.size

image

Copy link
Contributor Author

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;

I get:
lineheight

JSFiddle

Which seems just slightly off. I think the equation is missing something small or I'm doing it wrong.

@MPierre9
Copy link
Contributor Author

MPierre9 commented Feb 9, 2018

I think this resolves the slight offset
JSFiddle

lineheight3

@@ -857,11 +857,15 @@ module.exports = function(Chart) {

var label = itemToDraw.label;
if (helpers.isArray(label)) {
for (var i = 0, y = 0; i < label.length; ++i) {
var lineCount = label.length / 2;
Copy link
Member

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.

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;
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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.

var lineHeight = tickFont.size * 1.5;
var y = me.isHorizontal() ? 0 : -lineHeight * lineCount / 1.2;

for (var i = 0; i < label.length; ++i) {
Copy link
Member

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

@simonbrunel simonbrunel merged commit 584d1c6 into chartjs:master Feb 9, 2018
@simonbrunel simonbrunel added this to the Version 2.7.2 milestone Feb 9, 2018
@simonbrunel
Copy link
Member

Thanks @MPierre9

@MPierre9
Copy link
Contributor Author

Thanks for all the help @simonbrunel and @etimberg

dracos pushed a commit to mysociety/Chart.js that referenced this pull request Feb 26, 2018
@Moghul
Copy link

Moghul commented May 7, 2018

Thanks for the fix guys, I really appreciate it. Finally have time to try it out today :)

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

Successfully merging this pull request may close these issues.

4 participants