-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Click anywhere feature
#5443
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
Conversation
@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, 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
plotly.js/test/jasmine/tests/select_test.js
Line 132 in 8d7db15
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.
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!
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
Do I have to push the dist/ and build/ directories as well? They are not ignored.
I have added a test case. I consider this to be finished. Let me know if there are any more changes necessary.
Do I have to push the
dist/andbuild/directories as well? They are not ignored.
No. You should not commit them.
Tests don't seem to fail due to my changes, right?
@archmoj ?
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.
yiyuezhuo
commented
Feb 15, 2021
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.
Maybe worth it to add to the test case as well
try to enable click anywhere in Geo mode
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 :)
Failing tests seem unrelated to my additions. From my side this feature is ready to be merged @archmoj
Failing tests seem unrelated to my additions. From my side this feature is ready to be merged @archmoj
They all passed now.
Thanks very much for the PR and the follow up.
All looking good to me.
Passing over to @nicolaskruchten and @alexcjohnson.
You're welcome, looking forward to being able to finally start clicking anywhere!
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 ?
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.
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.
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.
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.
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.
Please see https://rreusser.github.io/plotly-mock-viewer/#plot_types as well as https://rreusser.github.io/plotly-mock-viewer/#polar_subplots which render https://github.com/plotly/plotly.js/blob/master/test/image/mocks/plot_types.json and
https://github.com/plotly/plotly.js/blob/master/test/image/mocks/polar_subplots.json.
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.
Apparently, there is also an issue with margins? #2696 (comment)
Don't have time right now to check
dmenne
commented
Jul 26, 2022
Any chance that this gets merged?
Pablongo24
commented
Nov 12, 2022
Can someone please summarize:
- Why this PR is still open?
- 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.
@Pablongo24 your comment comes across as quite aggressive and demanding and its tone is not particularly welcome here.
- Because there is still unaddressed feedback in the PR.
- 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 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).
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
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.
lmioduszewski
commented
Aug 4, 2024
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();
}
};
@drogers0
drogers0
Apr 16, 2025
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.
not sure if I implemented incorrectly, but to get this code to work in my case I changed part of it to
if(evt) { //no target unless you click on an object if(gd._hoverdata) { data = gd._hoverdata; } else { //the proposed object parsing did not work for me in the if statements or getting the coords var lat = gd._fullLayout.map._subplot.xaxis.p2c(); var lon = gd._fullLayout.map._subplot.yaxis.p2c(); data = [{lat: lat, lon: lon}]; }
I am using scattermap and plotly.js v3.0.0
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:
clickdemo