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

Add tooltip textLabelColor callback #4199

Merged
merged 7 commits into from May 28, 2017

Conversation

apoorvasrinivasan
Copy link
Contributor

@apoorvasrinivasan apoorvasrinivasan commented Apr 29, 2017

image

change the textLabelColor for charts

Fixes #4191

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

This is a good start. I think tests and documentation need to be added before this can be merged.

@@ -95,7 +95,8 @@ module.exports = function(Chart) {
// Args are: (tooltipItems, data)
beforeFooter: helpers.noop,
footer: helpers.noop,
afterFooter: helpers.noop
afterFooter: helpers.noop,
textLabelColor: helpers.noop
Copy link
Member

Choose a reason for hiding this comment

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

[nit] I would put this after labelColor since it's related to that stuff

Copy link
Member

Choose a reason for hiding this comment

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

Also, since this is defaulting to helpers.noop what is the default colour of the text? I feel like we should provide a default function that has the old behaviour

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes. i tried that, but the value of view.bodyFontColor at that moment returned to be black. instead of white so i reverted that to line 698. let me try something else.

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests. Just had one minor comment

@@ -87,6 +87,9 @@ module.exports = function(Chart) {
backgroundColor: view.backgroundColor
};
},
textLabelColor: function(tooltipItem, chart) {
return chart.tooltip._options.bodyFontColor;
Copy link
Member

Choose a reason for hiding this comment

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

it might be an idea here to pass the tooltip object as a parameter so that we're not reliant on it being a property of the chart. I know some users programmatically add extra tooltips and this would break that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etimberg can you please elaborate on this one. I'm not sure i understood that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i have updated the code using this.
since we need to pass the chart object, how to bypass the eslint error ?

@apoorvasrinivasan
Copy link
Contributor Author

@etimberg this error is because we need the chart in the function but we are using the tooltip to return the default value. how to bypass this?

@etimberg
Copy link
Member

@apoorvasrinivasan you could just remove the chart property on line 90 only (still pass it to the function but just don't make a parameter for it and it will be fine)

@apoorvasrinivasan
Copy link
Contributor Author

@etimberg yes, working now 👍

@apoorvasrinivasan
Copy link
Contributor Author

@etimberg is there something else i need to do ?

Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

@apoorvasrinivasan nothing needs to be done on my end. Just waiting for @simonbrunel to look as well

@@ -74,6 +74,7 @@ All functions are called with the same arguments: a [tooltip item](#chart-config
| `beforeLabel` | `tooltipItem, data` | Returns text to render before an individual label. This will be called for each item in the tooltip.
| `label` | `tooltipItem, data` | Returns text to render for an individual item in the tooltip.
| `labelColor` | `tooltipItem, chart` | Returns the colors to render for the tooltip item. [more...](#label-color-callback)
| `textLabelColor` | `tooltipItem, chart` | Returns the colors for the text of the label for the tooltip item.
Copy link
Member

Choose a reason for hiding this comment

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

What about labelTextColor instead?

@simonbrunel simonbrunel changed the title issue #4191 Add tooltip textLabelColor callback May 11, 2017
@simonbrunel simonbrunel added this to the Version 2.7 milestone May 11, 2017
@etimberg etimberg merged commit 394382b into chartjs:master May 28, 2017
exwm pushed a commit to exwm/Chart.js that referenced this pull request Apr 30, 2021
Add a new tooltip callback `labelTextColor` that returns the colour for each item in the body of the tooltip.

Fixes issue chartjs#4191
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

3 participants