Skip to content
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

Click anywhere feature #5443

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Click anywhere feature #5443

wants to merge 13 commits into from

Conversation

sleighsoft
Copy link
Contributor

This adds a click anywhere feature as proposed in #2696

This PR is meant as a point of discussion. Feel free to leave feedback.

Demo Code:

<!DOCTYPE html>
<html lang="en">

<head>
    <meta charset="UTF-8">
    <meta name="viewport" content="width=device-width, initial-scale=1.0">
    <script src="../plotly.js/dist/plotly.js"></script>
</head>

<body>
    <div id="tester" style="width:600px;height:250px;"></div>
    <script>
        var TESTER = document.getElementById('tester');
        Plotly.newPlot(TESTER, [{
            x: [1, 2, 3, 4],
            y: [4, 1, 3, 5],
            mode: "lines+markers",
            marker: {size: 12},
        }], {
            margin: { t: 0 },
            clickmode: "event+anywhere"
        });
        TESTER.on('plotly_click', function(clickData) {
            var x = clickData["points"][0]["x"];
            Plotly.relayout(TESTER, {
                shapes: [
                {
                    type: "line",
                    x0: x,
                    y0: 0,
                    x1: x,
                    y1: 1,
                    yref: "paper",
                    line: {color: "RoyalBlue", width:3},
                }
                ]
            });
        });
    </script>
</body>
</html>

clickdemo

@sleighsoft
Copy link
Contributor Author

@archmoj, I have a question regarding testing. I have looked through the code and did not find how you are testing the different clickmode flags under test\jasmine\tests\click_test.js. Could you give me some guidance/help on how to implement a proper test for it, please.

@archmoj
Copy link
Contributor

archmoj commented Jan 25, 2021

@archmoj, I have a question regarding testing. I have looked through the code and did not find how you are testing the different clickmode flags under test\jasmine\tests\click_test.js. Could you give me some guidance/help on how to implement a proper test for it, please.

The "select" clickmode are tested in `select_test"
For example

describe('Click-to-select', function() {

You could start a new test file perhaps anywhere_test to add new tests.

Also thanks for the new PR. It looks pretty close. And I don't think you need to close this one and reopen again now that you opened the PR from your click branch. If we ever wanted to merge upstream/master it should be easy to sync your repo.

@archmoj archmoj added status: in progress community community contribution feature something new labels Jan 25, 2021
@sleighsoft
Copy link
Contributor Author

Hey @archmoj, I have added a couple of test but I would like to debug them to make sure I didn't forget anything and that the expected values are set correctly. Do you have any recommendations on how I can debug a karma test? I've tried setting up the chrome debugging in vscode but wasn't successfull so far. It somehow never stops at my breakpoints even though they are at the start of the file. Any advice is very much welcome!

@archmoj
Copy link
Contributor

archmoj commented Jan 31, 2021

Hey @archmoj, I have added a couple of test but I would like to debug them to make sure I didn't forget anything and that the expected values are set correctly. Do you have any recommendations on how I can debug a karma test? I've tried setting up the chrome debugging in vscode but wasn't successfull so far. It somehow never stops at my breakpoints even though they are at the start of the file. Any advice is very much welcome!

Great news.
Please see https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md#using-the-developer-console-in-karma-to-writedebug-jasmine-tests

@sleighsoft
Copy link
Contributor Author

Do I have to push the dist/ and build/ directories as well? They are not ignored.

@sleighsoft
Copy link
Contributor Author

I have added a test case. I consider this to be finished. Let me know if there are any more changes necessary.

@archmoj
Copy link
Contributor

archmoj commented Feb 7, 2021

Do I have to push the dist/ and build/ directories as well? They are not ignored.

No. You should not commit them.

@sleighsoft
Copy link
Contributor Author

Tests don't seem to fail due to my changes, right?

@sleighsoft
Copy link
Contributor Author

@archmoj ?

@archmoj
Copy link
Contributor

archmoj commented Feb 10, 2021

Tests don't seem to fail due to my changes, right?

We experienced failures on the CircleCI in the last week.
Could you please fetch upstream/master and merge it into your branch?
That should fix the tests.

@archmoj archmoj requested a review from alexcjohnson February 10, 2021 23:43
@yiyuezhuo
Copy link

It seems that xaxis.p2d and yaxis.p2d doesn't exists in Map, gd._fullLayout.geo._subplot.xaxis.p2c() and gd._fullLayout.geo._subplot.yaxis.p2c() is an alternative to get LonLat coordinates.

@sleighsoft
Copy link
Contributor Author

Maybe worth it to add to the test case as well

yiyuezhuo and others added 2 commits February 16, 2021 19:17
@nicolaskruchten
Copy link
Contributor

A small update on timing: our team is working hard on releasing v2.0 of Plotly.js, which we anticipate will happen in early April. This PR would be a good candidate to land in the library in v2.1 or later, so with apologies for the delay, we will likely not be able to give much feedback on this PR for the next few weeks :)

@sleighsoft
Copy link
Contributor Author

sleighsoft commented Sep 11, 2021

Failing tests seem unrelated to my additions. From my side this feature is ready to be merged @archmoj

@archmoj
Copy link
Contributor

archmoj commented Sep 12, 2021

Failing tests seem unrelated to my additions. From my side this feature is ready to be merged @archmoj

They all passed now.

@archmoj
Copy link
Contributor

archmoj commented Sep 12, 2021

Thanks very much for the PR and the follow up.
All looking good to me.
Passing over to @nicolaskruchten and @alexcjohnson.

@archmoj archmoj added this to the v2.6.0 milestone Sep 12, 2021
@sleighsoft
Copy link
Contributor Author

You're welcome, looking forward to being able to finally start clicking anywhere!

@nicolaskruchten
Copy link
Contributor

Thanks for your contributions here @sleighsoft! We're hard at work on v2.6.0 but I want to get this merged into 2.7.0.

How/does this interact with double-clicks, out of curiosity? Do we need to think about any special handling here @alexcjohnson ?

if(gd._hoverdata) {
data = gd._hoverdata;
} else if(clickmode.indexOf('anywhere') > -1) {
if(gd._fullLayout.geo) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic means that if the plot has a geo subplot, we're assuming the click was on that, even if the plot has other subplots too. But we can do a lot better than that, and there are more subplot types than just geo and 2D cartesian. And fortunately we already have a mechanism to detect this: the third arg to click is subplot and it should tell us exactly which subplot the click came from.

2D cartesian, ternary, and polar subplots report this correctly (eg 'x2y3', 'ternary2', 'polar3'), geo doesn't but it should be easy to fix that. Then what we need to do is find the actual subplot object, and depending on its type calculate the appropriate coordinates within that particular subplot.

Pie, sankey, and funnelarea also all reach this point but don't give a subplot. We could have them give the trace number I guess, but is there anything useful to report for them? Just the raw coordinates within the plot?

3D, parcoords, and parcats don't even get here, I'm happy to ignore those for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that something that you intend to do in a second round or within this PR. If it is something I should fix, then I'd like a short example ideally for each plot type as I am not familiar with most of the ones you listed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexcjohnson
Copy link
Collaborator

How/does this interact with double-clicks, out of curiosity?

I'd need to play with it to really be able to say, but I'm not worried about it: this is being inserted right where the click event would have been emitted anyway, so if there's a problem with doubleclick it's already a problem without this feature.

As far as actually reporting a usable doubleclick for users, ATM our plotly_doubleclick event always returns null event data AFAICT, so if at some point we want to do better we can handle both hoverdata and anywhere-coordinates there.

@sleighsoft
Copy link
Contributor Author

Apparently, there is also an issue with margins? #2696 (comment)
Don't have time right now to check

@dmenne
Copy link

dmenne commented Jul 26, 2022

Any chance that this gets merged?

@Pablongo24
Copy link

Can someone please summarize:

  1. Why this PR is still open?
  2. Why it took until 2021 for someone to even start working on it? A click-anywhere feature seems so obvious to so many use cases. In the issues, people have been asking for it since Jan 2017 - #145.

@nicolaskruchten
Copy link
Contributor

@Pablongo24 your comment comes across as quite aggressive and demanding and its tone is not particularly welcome here.

  1. Because there is still unaddressed feedback in the PR.
  2. It's hard to answer such a question, and in some sense this is the question in open-source: there are thousands of people who in principle could have started working on it, but none of them had the right mix of time/motivation/skills to do so. If you step back a bit, it's actually more surprising that someone actually has done so: providing free labour to the rest of the world is the outlier behaviour rather than the expectation.

@Pablongo24
Copy link

Pablongo24 commented Nov 12, 2022

@Pablongo24 your comment comes across as quite aggressive and demanding and its tone is not particularly welcome here.

@nicolaskruchten not sure why it came across as aggressive. Just asked two questions out of legitimate curiosity. I understand the dynamics of working on open-source projects. Merely pointing out that in the 6 years since this issue first popped up, several other features have been added - many of them seemingly more difficult to implement (though I realize I'm not even remotely close to a JS expert, so my perception might be wrong).

@gvwilson gvwilson self-assigned this Jun 10, 2024
@gvwilson
Copy link
Contributor

This pull request has been sitting for a while, so I would like to close it as part of our effort to tidy up our public repositories. I've assigned it to myself to keep track of it; I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then. Thanks - @gvwilson

@nve-sak
Copy link

nve-sak commented Jun 24, 2024

As a Python user of Plotly, I'll toss in that capturing the coordinates of a click is a desirable feature with considerable downstream potential. Unfortunately, I don't have the plotly.js (or any javascript) knowledge to be of any help. Thanks a lot to the Plotly team for working hard on javascript, Python and other bindings.

@gvwilson gvwilson removed their assignment Aug 2, 2024
@lmioduszewski
Copy link

If it's helpful to anyone else...I added the ability to get lat/lon from a mapbox using the above click anywhere code. Code for the click.js file is below. My fork is here https://github.com/lmioduszewski/plotly.js/tree/Click-Anywhere

I also made edits to my own fork of plotly.py so it works in Python as well. That branch is here https://github.com/lmioduszewski/plotly.py.

Both of these are many commits behind, and I'm not sure the best way to update my repos to be consistent with the current plotly.py/plotly.js.

use strict';

var Registry = require('../../registry');
var hover = require('./hover').hover;

module.exports = function click(gd, evt, subplot) {
    var annotationsDone = Registry.getComponentMethod('annotations', 'onClick')(gd, gd._hoverdata);

    // fallback to fail-safe in case the plot type's hover method doesn't pass the subplot.
    // Ternary, for example, didn't, but it was caught because tested.
    if(subplot !== undefined) {
        // The true flag at the end causes it to re-run the hover computation to figure out *which*
        // point is being clicked. Without this, clicking is somewhat unreliable.
        hover(gd, evt, subplot, true);
    }

    function emitClick(data) { gd.emit('plotly_click', {points: data, event: evt}); }

    var clickmode = gd._fullLayout.clickmode;
    var data;
    if(evt && evt.target) {
        if(gd._hoverdata) {
            data = gd._hoverdata;
        } else if(clickmode.indexOf('anywhere') > -1) {
            if(gd._fullLayout.geo) {
                var lat = gd._fullLayout.geo._subplot.xaxis.p2c();
                var lon = gd._fullLayout.geo._subplot.yaxis.p2c();
                data = [{lat: lat, lon: lon}];
            } if(gd._fullLayout.mapbox) {
                var mapboxInstance = gd._fullLayout.mapbox._subplot.map;
                var rect = evt.target.getBoundingClientRect();
                var xx = evt.clientX - rect.left;
                var yy = evt.clientY - rect.top;
                var lngLat = mapboxInstance.unproject([xx, yy]);
                data = [{lon: lngLat.lng, lat: lngLat.lat}];
            } else {
                var bb = evt.target.getBoundingClientRect();
                var x = gd._fullLayout.xaxis.p2d(evt.clientX - bb.left);
                var y = gd._fullLayout.yaxis.p2d(evt.clientY - bb.top);
                data = [{x: x, y: y}];
            }
        }
        if(data) {
            if(annotationsDone && annotationsDone.then) {
                annotationsDone.then(function() { emitClick(data); });
            } else emitClick(data);
        }

        // why do we get a double event without this???
        if(evt.stopImmediatePropagation) evt.stopImmediatePropagation();
    }
};

@gvwilson gvwilson added P2 considered for next cycle and removed status: has TODOs labels Aug 8, 2024
@gvwilson gvwilson self-assigned this Nov 20, 2024
@gvwilson gvwilson added the cs customer success label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution cs customer success feature something new P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.