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

Display tooltip nearest cursor on non-bar chart #2476

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@ajb32x
Copy link

ajb32x commented Sep 13, 2018

I want to resolve 2362 2391, this displays the tooltip for the closest point by horizontal distance to the cursor.
fixes #2391

@kt3k

This comment has been minimized.

Copy link
Member

kt3k commented Sep 16, 2018

The preview example charts (which includes this branch's change) are available here, but it doesn't seem fixing the problem reported in #2362.

src/data.js Outdated
ChartInternal.prototype.findClosest = function (values, pos) {
var $$ = this,
minDist = $$.config.point_sensitivity,
minDist,

This comment has been minimized.

@danidin

danidin Sep 16, 2018

Contributor

Seems like minDist is never defined

This comment has been minimized.

@ajb32x

ajb32x Sep 17, 2018

Author

It is defined later, on line 357.

@ajb32x

This comment has been minimized.

Copy link
Author

ajb32x commented Sep 17, 2018

The preview example charts (which includes this branch's change) are available here, but it doesn't seem fixing the problem reported in #2362.

You are right. I was working on the issue #2391, but that was closed as a duplicate of #2362. It looks like they may not actually be duplicates.

@ajb32x ajb32x changed the title Issue 2362 - Display tooltip nearest cursor on non-bar chart Display tooltip nearest cursor on non-bar chart Sep 17, 2018

Show resolved Hide resolved src/data.js Outdated
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Sep 27, 2018

Codecov Report

Merging #2476 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2476      +/-   ##
==========================================
+ Coverage   80.48%   80.51%   +0.02%     
==========================================
  Files          54       54              
  Lines        4279     4285       +6     
==========================================
+ Hits         3444     3450       +6     
  Misses        835      835
Impacted Files Coverage Δ
src/data.js 86.12% <100%> (+0.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87bbfb1...b3cc67b. Read the comment docs.

@ifokeev

This comment has been minimized.

Copy link

ifokeev commented Oct 11, 2018

merge please

@9teen90nine

This comment has been minimized.

Copy link

9teen90nine commented Oct 11, 2018

need this merged too!

@arnage

This comment has been minimized.

Copy link

arnage commented Oct 11, 2018

great! merge it please

@adamkasprowicz

This comment has been minimized.

Copy link

adamkasprowicz commented Oct 12, 2018

:D btw if you can't wait for the merge - change this part of the code manually in node_modules. Works fine 👍 *unless you're building your bundle in CI pipeline

@ifokeev

This comment has been minimized.

Copy link

ifokeev commented Oct 15, 2018

I found that PR doesn't resolve tooltip showing correctly.
I noticed point_sensitivity config variable that isn't documented.
You have to set

        point: {
          sensitivity: 150
        },

and it will be actually the same behaviour as in the previous versions

@ajb32x

This comment has been minimized.

Copy link
Author

ajb32x commented Oct 15, 2018

I found that PR doesn't resolve tooltip showing correctly.
I noticed point_sensitivity config variable that isn't documented.
You have to set

        point: {
          sensitivity: 150
        },

and it will be actually the same behaviour as in the previous versions

This PR should fix the issue in #2391 , which looks to have been mistakenly marked as a duplicate of #2362 . I'm happy to make changes, I would like issue #2391 resolved.

My changes actually remove the use of the point_sensitivity variable for non bar charts. I don't believe it is necessary to fix #2391 .

@vlarkovich

This comment has been minimized.

Copy link

vlarkovich commented Oct 16, 2018

The preview example charts (which includes this branch's change) are available here, but it doesn't seem fixing the problem reported in #2362.

It seems that this PR will break the functionality of the tooltip when it is displayed separately for each point.
Try to open not grouped tooltip, scatter.

@hln

This comment has been minimized.

Copy link

hln commented Nov 2, 2018

@vlarkovich is correct. When tooltip.grouped is set to false, the tooltip no longer works without having to directly hover over the points.

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