-
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
Configurable Tooltip Position Modes #3453
Conversation
@@ -214,6 +214,7 @@ enabled | Boolean | true | Are tooltips enabled | |||
custom | Function | null | See [section](#advanced-usage-external-tooltips) below | |||
mode | String | 'nearest' | Sets which elements appear in the tooltip. See [Interaction Modes](#interaction-modes) for details | |||
intersect | Boolean | true | if true, the tooltip mode applies only when the mouse position intersects with an element. If false, the mode will be applied at all times. | |||
positionMode | String | 'average' | The mode for positioning the tooltip. 'average' mode will place the tooltip at the average position of the items displayed in the tooltip. 'nearest' will place the tooltip at the position of the element closest to the event 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.
What do you think about simply use position
as option name?
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.
That sound better
/** | ||
* @namespace Chart.Tooltip.modes | ||
*/ | ||
Chart.Tooltip.modes = { |
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 would choose another name because it can be confusing with the tooltip.mode
option (not sure what name to pick then, maybe Chart.Tooltip.positioners
).
} | ||
|
||
// Hover animations | ||
if (!me.animating) { | ||
// If entering, leaving, or changing elements, animate the change via pivot | ||
if (!helpers.arrayEquals(me.active, me.lastActive) || |
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.
Does that mean the chart is redrawn at every mouse move? If so, that's not good :)
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.
Unfortunately, yes. What do you think about detecting if there are changes in update and returning a Boolean. If there are changes, we redraw.
Alternatively, we factor the position code out to here and pass it to the tooltip before update.
👍
|
I changed I still need to figure out how to better do the updates so that we do not continuously redraw. |
@simonbrunel I followed your advice and split up the We now do var changed = me.handleEvent(e);
changed |= me.legend.handleEvent(e);
changed |= me.tooltip.handleEvent(e); then only re-render if we changed. The |
@simonbrunel have you had a chance to review the latest changes I made? |
Adds new tooltip position option that allows configuring where a tooltip is displayed on the graph in relation to the elements that appear in it
Created the concept of tooltip modes. User defined modes are supported by changing
Chart.Tooltip.modes
map.The default mode is 'average'. We may consider changing this if we think the old behaviour is really a bug that no one wanted. Currently. this will not change existing charts.
Also implemented is the 'nearest' mode which places the tooltip on the element closest to the event that triggered the tooltip. Implementing this required removing some of the optimizations from
ChartController#eventHandler
. This may not be acceptable so there may need to be some refactoring.This addresses #2056
CC @chartjs/maintainers