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

Fixes Too Many Datapoints being hovered #5332

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@MPierre9
Contributor

MPierre9 commented Mar 12, 2018

Fixes #5231

While investigating core.interaction.js I noticed the culprit of this problem is xRange(mouseX). The reasoning behind this is when there are an excessive amount of data points more than one falls into the range of x causing there to be multiple datapoints hovered because they are so close together. Of course, when there are fewer data points (around 40 or less) this doesn't occur.

To fix this I made a separate function called xRangeSmall that narrows down the range of which a data point will be retrieved. In my testing, it only occurred when there was more than 40 data points on the graph so I added an else if to x: function(chart, e, options) that accounts for this problem when there are more than 40 data points. Feedback is welcome!

Result

chart tooltip fix

JSFiddle

MPierre9 added some commits Mar 12, 2018

if (element.inXRange(position.x)) {
items.push(element);
}
} else if (element.datasetlength >= 40) {

This comment has been minimized.

@benmccann

benmccann Apr 7, 2018

Collaborator

you could turn this else if into simply an else

however, i'm not sure why the check for whether there are more or less than 40 elements is being added. Why not always to do the cleanup of extra elements?

This comment has been minimized.

@MPierre9

MPierre9 Apr 7, 2018

Contributor

You're right the else if check is kind of redundant. After some extensive testing, I found the issue occurs after there are 40 datapoints on the chart. From my understanding, the reason for inXRange is to allow the user to not have to be precisely on the 'x' coordinate to have the datapoint highlighted by the tooltip and rather have the user be in a certain range of the tooltip making it easier to highlight. However, the main problem here is when we have say 300+ datapoints there begins to be too many data points that fall within that same range. Overall the having a separate function inXRangeSmall narrows the range in which a datapoint will be returned if we have an extensive amount of datapoints (40+). Also I can add the cleanup to both just in case. Thanks for the review!

This comment has been minimized.

@MPierre9

MPierre9 Apr 8, 2018

Contributor

When I convert the else if to else I get the "no-lonely-if" lint error.

chartjsif

This comment has been minimized.

@benmccann

benmccann Apr 8, 2018

Collaborator

It wants you to combine the else with the next if: } else iif (element.inXRangeSmall(position.x)) {

I still am not understanding though why we do not always call inXRangeSmall and remove the check for whether there are 40 datapoints

@MPierre9

This comment has been minimized.

Contributor

MPierre9 commented Apr 8, 2018

@benmccann I implemented your suggestion (remove the check for 40 datapoints) and transferred the functionality of inXRangeSmall to inXRange the code essentially functions the same from my testing, however, I think the tests performed with gulp test have to be changed to account for the new X range. For example: https://github.com/chartjs/Chart.js/blob/master/test/specs/core.interaction.tests.js#L579 seems to push an error with the new X range.

@@ -37,6 +37,7 @@ function parseVisibleItems(chart, handler) {
for (j = 0, jlen = meta.data.length; j < jlen; ++j) {
var element = meta.data[j];
if (!element._view.skip) {
element.datasetlength = meta.data.length;

This comment has been minimized.

@benmccann

benmccann Apr 8, 2018

Collaborator

this line seems unused

@@ -24,7 +24,7 @@ defaults._set('global', {
function xRange(mouseX) {
var vm = this._view;
return vm ? (Math.abs(mouseX - vm.x) < vm.radius + vm.hitRadius) : false;
return vm ? (Math.abs(mouseX - vm.x) < 2) : false;

This comment has been minimized.

@benmccann

benmccann Apr 8, 2018

Collaborator

why 2?

This comment has been minimized.

@MPierre9

MPierre9 Apr 9, 2018

Contributor

After testing I found 2 to be a reasonable number to have a decent X range without returning too many tooltip items. I found the lower the number the more precise the user has to be to the X coordinate of the datapoint for it to be highlighted which can get tedious if you have say 10 data points and are trying to have one highlighted.

@benmccann

This comment has been minimized.

Collaborator

benmccann commented Jun 28, 2018

Closing in favor of #5578

@benmccann benmccann closed this Jun 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment