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

Multi-line rotated line-height changes on upgrade from 2.4.0 to 2.5.0 #4379

Closed
tntim96 opened this issue Jun 15, 2017 · 8 comments · Fixed by #5914
Closed

Multi-line rotated line-height changes on upgrade from 2.4.0 to 2.5.0 #4379

tntim96 opened this issue Jun 15, 2017 · 8 comments · Fixed by #5914
Milestone

Comments

@tntim96
Copy link

tntim96 commented Jun 15, 2017

Graph with rotated labels on 2.4.0 works well:
https://codepen.io/anon/pen/xrRVRw

But after upgrading to 2.6.0 (2.5.0 seems to have the issue too), the line-height of the text has increased significantly:
https://codepen.io/anon/pen/yXVOVw

A smaller default or configurable line height would be handy. Might be related to #4292

@tntim96
Copy link
Author

tntim96 commented Jun 16, 2017

The line spacing can be changed by altering https://github.com/chartjs/Chart.js/blob/master/src/core/core.scale.js#L723, changing:

	// apply same lineSpacing as calculated @ L#320
	y += (tickFontSize * 1.5);

...to...

	// apply same lineSpacing as calculated @ L#320
	y += (tickFontSize * 1.0);

I'll look into this to see what that change may break and what L#320 is - on line 310 at the time this code was committed, the code is:

	if (isHorizontal) {
		minSize.height += (scaleLabelFontSize * 1.5);
	} else {
		minSize.width += (scaleLabelFontSize * 1.5);
	}

...which looks relevant. Looks like that code has since changed too.

@tntim96
Copy link
Author

tntim96 commented Jun 16, 2017

y += (tickFontSize * 1.5); added here

Need to work out why the 1.5 was chosen in the first place and whether it was made redundant at some stage. If the solution is to change to the factor to 1.0, the multiplication can be removed altogether. Another approach could be to make the factor configurable, but it seems like line-height should be able to be calculated.

@etimberg
Copy link
Member

Line height is an interesting idea for this. I'm happy to look at a PR for it. @simonbrunel thoughts?

@simonbrunel
Copy link
Member

Agree, all multi lines text should support configurable line height.

@tntim96
Copy link
Author

tntim96 commented Jun 27, 2017

I'm happy to have a go at this but am a bit new to this project and node. I can see the coverage tests don't hit the line in question, an would like to add a test. How do you develop and debug the tests? Do you write them in a browser, work out what assertions to make, then paste them in a related jasmine spec?

@tntim96
Copy link
Author

tntim96 commented Jun 27, 2017

K - am able to debug using the browser with gulp unittest --watch. Will see how I go from there.

@jamesonnuss
Copy link

Any update on this? @tntim96 Thanks

@binDongKim
Copy link

Are we able to adjust the line-height of the ticks especially when they are multi lines?

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

Successfully merging a pull request may close this issue.

5 participants