-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Conversation
There was a problem hiding this 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.
src/core/core.tooltip.js
Outdated
@@ -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 |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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
src/core/core.tooltip.js
Outdated
@@ -87,6 +87,9 @@ module.exports = function(Chart) { | |||
backgroundColor: view.backgroundColor | |||
}; | |||
}, | |||
textLabelColor: function(tooltipItem, chart) { | |||
return chart.tooltip._options.bodyFontColor; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
@etimberg this error is because we need the |
@apoorvasrinivasan you could just remove the |
@etimberg yes, working now 👍 |
@etimberg is there something else i need to do ? |
There was a problem hiding this 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
docs/configuration/tooltip.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about labelTextColor
instead?
Add a new tooltip callback `labelTextColor` that returns the colour for each item in the body of the tooltip. Fixes issue chartjs#4191
change the textLabelColor for charts
Fixes #4191