-
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
Custom tooltip: add data points infos #3201
Conversation
Looks good :) I would also have updated the custom tooltips example to show how to use this new dataPoints property. |
Hi @simonbrunel, Instead of updating example, I added a new one under Not sure about the naming though ^^' |
@bydooweedoo name is fine by me |
Good for me as well |
Err, the reason people put trailing commas into code is so to :D you just added more, by removing a comma that was there to avoid diff I'm sure there is a programmer joke hiding in this situation... On Mon, Sep 12, 2016 at 4:01 PM, Simon Brunel [email protected]
|
Hello @rjmcguire , I use trailing comma usually, but here it makes Sorry about that |
Hi @bydooweedoo, I meant no offence or correction it just reminded me of an xkcd comic so I thought to share in the hopes it would humour you as much as it did me. |
@rjmcguire FYI, we just changed it :) |
@@ -159,6 +159,19 @@ var myPieChart = new Chart(ctx, { | |||
}); | |||
``` | |||
|
|||
In addition to all tooltip properties, a `dataPoints` property is added containing an `array[CustomTooltipDataPoint]`. | |||
|
|||
#### CustomTooltipDataPoint |
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 think we already have a section on TooltipItems in the docs (since they are exposed to other callbacks). Can we link to that section instead of having it duplicated?
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.
Sorry for the delay, I move this documentation from Advanced to proper Chart Configuration part.
…nfiguration section
datasetIndex | Number | Matching dataset index. | ||
xLabel | String | Matching label on X axis. | ||
yLabel | String | Matching label on Y axis. | ||
pointX | Number | X position of matching point. |
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.
Just thinking, while the containing object isdataPoints
, wouldn't x
and y
be better names?
dataPoints.forEach(function(point) {
point.x; // compared to point.pointX
point.y; // compared to point.pointY
});
Would also be more consistent with index
(and not pointIndex
).
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 think you are right :)
It makes more sense. I will change this.
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.
Other than my 1 comment on the docs, this looks good
@@ -257,6 +257,18 @@ afterBody | `Array[tooltipItem], data` | Text to render after the body section | |||
beforeFooter | `Array[tooltipItem], data` | Text to render before the footer section | |||
footer | `Array[tooltipItem], data` | Text to render as the footer | |||
afterFooter | `Array[tooltipItem], data` | Text to render after the footer section | |||
dataPoints | `Array[CustomTooltipDataPoint], data` | List of matching point informations. | |||
|
|||
#### CustomTooltipDataPoint |
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 believe the section immediately below this describes the same thing. At a minimum we should add the new x
and y
properties to that code snippet as well. Maybe we should consider merging these sections as well.
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.
Hello @etimberg ,
I am not sure what you want me to do here. Could you explain more ?
Which sections do you want to merge ? If you are talking about CustomTooltipDataPoint
and the one just before, dataPoints
is an array so it should be defined as an external section right ?
Also for x
and y
, which code snippet are you talking about ?
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 meant the snippet here https://github.com/chartjs/Chart.js/blob/master/docs/01-Chart-Configuration.md#tooltip-item-interface
I was also thinking to merge with the same section to that 2nd column of the last line of the table becomes Array[TooltipItemInterface]
. In other words, merging the CustomTooltipDataPoint
and TooltipItemInterface
sections since they describe the same thing.
Also, for the tooltip item do we need the x
and y
properties? the user can look them up if necessary.
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.
Alright, yes up to you we can merge the two interface together.
I didn't know TooltipItemInterface
and the CustomTooltipDataPoint
were referring to the same thing, that's why I separate the two in the first place.
In my opinion, x
and y
properties are usefull. I am using them in several of my projects to be able to show individual tooltips at each points position.
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.
Ah, ok. That's my bad, I should have been clearer. If you can merge the doc sections and fix the merge conflict I'll merge this
Conflicts: src/core/core.tooltip.js
Hi @etimberg , I merged with current master. No more conflict. Let me know if anything. |
Expose tooltip items from tooltip model and added `x` and `y` properties to `TooltipItemInterface`
Expose tooltip items from tooltip model and added `x` and `y` properties to `TooltipItemInterface`
Hello everyone,
Just added more informations to the custom tooltip callback argument.
In order to know which data points are matching, I added a new property
dataPoints
to the tooltip object passed to the custom tooltip.This property contains an
array[CustomTooltipDataPoint]
describes as following:CustomTooltipDataPoint
I also added a test case covering this new property, and a description inside the documentation under the Advanced External Tooltips section.
Please let me know if anything,
Thanks