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

allow multiple lines of text in the line label #129

Merged
merged 8 commits into from Apr 30, 2019

Conversation

aoifef
Copy link
Contributor

@aoifef aoifef commented Jul 23, 2018

screen shot 2018-07-23 at 15 28 29

Using "\n" in the label content will allow the user to take new lines for the line label.

@mheiduk
Copy link

mheiduk commented Aug 23, 2018

Would be great if this change can be merged!

@zreese
Copy link

zreese commented Sep 17, 2018

omg someone please merge this. 😓

@aoifef aoifef changed the title adding changes to allow multiple lines of text in the line label by u… allow multiple lines of text in the line label Sep 18, 2018
@alex2wong
Copy link

@zreese @mheiduk it seems ... Nobody is actively maintaining this repo.. few pr have been merged since this year. @benmccann could you pls take a look at the prs. thanks a lot : )

@ffoxwell
Copy link

This functionality would be really beneficial, could we please get this merged?

src/types/line.js Outdated Show resolved Hide resolved
src/types/line.js Outdated Show resolved Hide resolved
src/types/line.js Outdated Show resolved Hide resolved
src/types/line.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Collaborator

@alex2wong I am not a maintainer of this repo and do not have merge rights. You could try the #dev Slack channel for Chart.js perhaps

src/types/line.js Outdated Show resolved Hide resolved
@simonbrunel
Copy link
Member

@benmccann I added you as collaborator to this repository (I thought it was already the case).

@ All: we are looking for new maintainers for the annotation and zoom plugins, so if you are interested, please reach us on the #dev room of our Slack.

view.labelY + (view.labelHeight / 2)
);

if (view.labelContent !== null && view.labelContent.indexOf("\n") !== -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need this if/else check? view.labelContent.split("\n") will return an array with a single element and then it should be able to be processed just like if there were multiple elements

textWidth = ctx.measureText(longestLabel).width;

model.labelHeight = (textHeight * labelContentArray.length) + (2 * model.labelYPadding);
//Add padding for in between each label item
Copy link
Collaborator

Choose a reason for hiding this comment

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

add a space after // and remove extraneous word "for"

textYPosition
);

textYPosition += (view.labelFontSize + 5);
Copy link
Collaborator

Choose a reason for hiding this comment

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

these parentheses are unnecessary


model.labelHeight = (textHeight * labelContentArray.length) + (2 * model.labelYPadding);
//Add padding for in between each label item
model.labelHeight += 5 * (labelContentArray.length - 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this padding should probably be configurable. and perhaps the default should be something like fontSize / 3

@benmccann benmccann closed this Dec 14, 2018
@benmccann benmccann reopened this Dec 14, 2018
@benmccann
Copy link
Collaborator

@aoifef I setup a gulp lint task so that you can run the style checks manually locally to speed up the review cycles. Hope this helps. The new Travis setup is currently failing with this PR and will need to be fixed before we can merge

@benmccann
Copy link
Collaborator

@aoifef just a reminder that you will need to make gulp lint pass

@siemendes
Copy link

Hello,
is it possible to merge the solution?
In developpement with the correction, it's perfect for me.
But i have a bug when i build my project for production with the latest version.
Thanks a lot :-)

@benmccann
Copy link
Collaborator

The main Chart.js repo typically takes input as an array for multiple lines instead of using \n. It'd be good to do the same here for consistency

var textHeight = model.labelFontSize;
model.labelHeight = textHeight + (2 * model.labelYPadding);

if (model.labelContent !== null && model.labelContent.indexOf('\n') !== -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like it would fail if labelContent is undefined. Maybe just check model.labelContent rather than model.labelContent !== null

@aoifef
Copy link
Contributor Author

aoifef commented Apr 2, 2019

The main Chart.js repo typically takes input as an array for multiple lines instead of using \n. It'd be good to do the same here for consistency

@benmccann could you provide an example of this or point me in the right direction in the Chart.js repo?

src/types/line.js Outdated Show resolved Hide resolved
@benmccann
Copy link
Collaborator

@aoifef
Copy link
Contributor Author

aoifef commented Apr 30, 2019

Changes applied, evidence of working functionality:
Screenshot 2019-04-30 at 15 44 52


model.labelHeight = (textHeight * model.labelContent.length) + (2 * model.labelYPadding);
// Add padding in between each label item
model.labelHeight += 5 * (model.labelContent.length - 1);
Copy link
Collaborator

@benmccann benmccann Apr 30, 2019

Choose a reason for hiding this comment

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

can we use model.labelYPadding instead of hard coding 5? (same below on line 282)

@benmccann benmccann merged commit fdafa46 into chartjs:master Apr 30, 2019
@mtskf
Copy link

mtskf commented Jun 21, 2019

This is not working...

@benmccann
Copy link
Collaborator

benmccann commented Jun 24, 2019

@mtskf are you using the code from master or the latest release? This code hasn't been included in a release yet. If you are using the code from master, it'd be helpful to know in what way it's not working

osro added a commit to osro/chartjs-plugin-annotation that referenced this pull request Jan 28, 2020
* commit '7037eea71314613a8af83d2298241841cbc34c4f':
  Fix undefined reference
  "preserveComments" not longer available on gulp-uglify (chartjs#177)
  Added PLUGIN ID because missing (chartjs#173)
  allow multiple lines of text in the line label (chartjs#129)
  Chart.js should be a peerDependency
  Fix getting chart borders for box size setting (chartjs#152)
  Fix a number of lint errors in the samples
  Add missing .htmllintrc
  Lint sample html (chartjs#150)
  Pass the index of value datapoint to getPixelValue (chartjs#109)
  move configure step to draw time, to support render call (chartjs#116)
  Read plugin options from plugins.annotation (chartjs#130)
  Add Travis config
  Add gulp lint task (chartjs#144)
  Fix Dropbox url

# Conflicts:
#	README.md
#	package.json
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