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

Update doc on label types for TooltipItem #6030

Merged
merged 2 commits into from Feb 1, 2019

Conversation

nagix
Copy link
Contributor

@nagix nagix commented Jan 30, 2019

label and value in TooltipItem are the return values from scale.getLabelForIndex which can be number or string.

Fixes #5830

etimberg
etimberg previously approved these changes Jan 30, 2019
kurkle
kurkle previously approved these changes Jan 30, 2019
benmccann
benmccann previously approved these changes Jan 30, 2019
@simonbrunel simonbrunel added this to the Version 2.8 milestone Jan 30, 2019
@simonbrunel
Copy link
Member

@nagix @kurkle shouldn't we instead force label and value to be strings?

At some point we will introduce a formatter to be able to suffix the value (e.g. 42 -> '42%') in the tooltip, so to me it would make sense that these tooltip item label and value are always strings.

@benmccann
Copy link
Contributor

I was debating that as well. That approach would make sense to me

@kurkle
Copy link
Member

kurkle commented Jan 30, 2019

I agree they should be strings. That is not the case (yet) though, and these changes reflect current situation.
When/if I get proposal for formatter option done, these can be re-adjusted in that PR (or whatever PR that provides the conversion to strings).

@simonbrunel
Copy link
Member

If users expect label and value to be numbers (thus to be the original data), then it may be a breaking change to automatically convert it to string later. Since it's 2 new properties, it may be better to force a string before releasing 2.8?

@kurkle
Copy link
Member

kurkle commented Jan 30, 2019

For these new ones, yes I think they should be formatted and/or forced to string before 2.8 release.
Those old ones will most likely have to stay as they are.
.. so maybe forcing these new ones to string right now would not be a bad start.

@nagix nagix dismissed stale reviews from benmccann, kurkle, and etimberg via 1b956ce January 31, 2019 08:17
@nagix
Copy link
Contributor Author

nagix commented Jan 31, 2019

Now createTooltipItem sets label and value to string values. I have also added a few tests.

Copy link
Member

@kurkle kurkle left a comment

Choose a reason for hiding this comment

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

Kudos for adding tests!

@simonbrunel simonbrunel merged commit 80a159e into chartjs:master Feb 1, 2019
kurkle pushed a commit to kurkle/Chart.js that referenced this pull request Feb 2, 2019
Also update the docs for `xLabel` and `yLabel` to also accept a `number`.
kurkle pushed a commit to kurkle/Chart.js that referenced this pull request Feb 2, 2019
Also update the docs for `xLabel` and `yLabel` to also accept a `number`.
@nagix nagix deleted the issue-6030 branch February 3, 2019 16:58
janelledement pushed a commit to janelledement/Chart.js that referenced this pull request Feb 10, 2019
Also update the docs for `xLabel` and `yLabel` to also accept a `number`.
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

5 participants