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

Multiple lines of text in the chart title #4382

Merged
merged 1 commit into from Jun 19, 2017
Merged

Multiple lines of text in the chart title #4382

merged 1 commit into from Jun 19, 2017

Conversation

etimberg
Copy link
Member

Adds support for multiple lines of text in the chart title. If the text property of the title options is an array, we render each item on it's own line. See attached screenshots for details of how it looks.

screen shot 2017-06-15 at 6 55 06 pm

screen shot 2017-06-15 at 6 55 18 pm

screen shot 2017-06-15 at 6 55 31 pm

@etimberg etimberg added this to the Version 2.7 milestone Jun 15, 2017
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.

Actually, I'm going to refactor and normalize multi line support since I implemented it for the datalabels plugin (will import common code in core, so may overwrite parts of this PR). Will support array of strings, but also string containing \n (array of array of string with carriage return also supported :) ).

lineHeight seems more standard and in line with css.

@@ -13,6 +13,7 @@ module.exports = function(Chart) {
weight: 2000, // by default greater than legend (1000) to be above
fontStyle: 'bold',
padding: 10,
lineSpacing: 5,
Copy link
Member

Choose a reason for hiding this comment

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

lineHeight?

Copy link
Member

Choose a reason for hiding this comment

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

Default null? (use fontSize)

minSize = me.minSize;
minSize = me.minSize,
lineCount = helpers.isArray(opts.text) ? opts.text.length : 1,
textSize = display ? (lineCount * fontSize) + (opts.padding * 2) + (opts.lineSpacing * (lineCount - 1)) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

By default, lineHeight should be equal to fontSize, then textSize is lineHeight*lineCount. If lineHeight is 0, all lines draw on the same line.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, that makes sense. Will update.

@etimberg
Copy link
Member Author

@simonbrunel sounds good about the multi-line support. Do we want to merge this first or after your changes?

@etimberg
Copy link
Member Author

Line height will also resolve #2747

@simonbrunel
Copy link
Member

We can merge this first, will review all line height usages in a future PR.

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.

None yet

2 participants