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

Configurable doubleclick #3991

Merged
merged 11 commits into from
Jul 22, 2019

Conversation

pynklu
Copy link

@pynklu pynklu commented Jun 27, 2019

Add an option 'doubleClickDelay' to the layout object (0 - 1000ms), to optionally overwrite the default 300ms doubleclick delay
#2206

first time making a PR (and don't really work with git), so sorry in advance if i did something wrong :-)

@etpinard
Copy link
Contributor

Thanks very much for the PR!

I think doubleClickDelay would be better off as config option, unless @plotly/plotly_js opposes.

We'll also need to make all subplots including the ones that don't use dragElement (e.g. geo and mapbox), use the set double-click value.

Thanks again, let me know if you any other questions.

@pynklu
Copy link
Author

pynklu commented Jun 28, 2019

Ah yes, that makes more sense. Can I add extra commits to this PR, or?

@pynklu
Copy link
Author

pynklu commented Jun 28, 2019

Alright, I changed it to use the config object. I went through the geo & mapbox code & couldn't find anything doublclick related that doesn't come from dragelem/ or legend/
Couldn't find any double-click behaviour that didn't use the adjusted delay either through manual testing

@etpinard
Copy link
Contributor

etpinard commented Jul 2, 2019

I went through the geo & mapbox code & couldn't find anything doublclick related that doesn't come from dragelem/ or legend/

See

bgRect.on('dblclick.zoom', zoomReset);

which uses the d3 double click handler

map.on('dblclick', function() {
var optsNow = gd._fullLayout[self.id];
Registry.call('_storeDirectGUIEdit', gd.layout, gd._fullLayout._preGUI, self.getViewEdits(optsNow));
var viewInitial = self.viewInitial;
map.setCenter(convertCenter(viewInitial.center));
map.setZoom(viewInitial.zoom);
map.setBearing(viewInitial.bearing);
map.setPitch(viewInitial.pitch);
var viewNow = self.getView();
optsNow._input.center = optsNow.center = viewNow.center;
optsNow._input.zoom = optsNow.zoom = viewNow.zoom;
optsNow._input.bearing = optsNow.bearing = viewNow.bearing;
optsNow._input.pitch = optsNow.pitch = viewNow.pitch;
gd.emit('plotly_doubleclick', null);
gd.emit('plotly_relayout', self.getViewEdits(viewNow));
});

which uses mapbox-gl's double click handler


There might be a way to make those two handlers adhere to gd._context.doubleClickDelay, but I'd be ok punting these to #2057

@etpinard
Copy link
Contributor

etpinard commented Jul 2, 2019

Now @pynklu would you be interested in trying to:

  • make npm run test-jasmine --bundleTest=requirejs_test.js --nowatch pass
  • add a new test similar to this one in the click_test.js suite

@pynklu
Copy link
Author

pynklu commented Jul 2, 2019

afbeelding
doesn't this imply that test passes?

@etpinard
Copy link
Contributor

etpinard commented Jul 2, 2019

@pynklu
Copy link
Author

pynklu commented Jul 3, 2019

@etpinard sorry, but I can't get the tests to run properly locally on my end + really don't know how to properly write new tests. Would you mind taking this up? :-)
Let me know if there's anything else I can help out with

@etpinard
Copy link
Contributor

etpinard commented Jul 18, 2019

TODO (that hopefully I'll have time to do before 1.49.0 is ready):

  • add 1 test in config_test.js, not needed as the logic is trivial
  • add a few dragelement_test.js tests
  • add a few legend tests
  • add note in doubleClickDelay plot_config.js description about limitations i.e. it has no effect on geo and mapbox subplot double-click delays for now.

@etpinard
Copy link
Contributor

cc @plotly/plotly_js this PR is now rewiewable and tagged under 1.49.0 (why not right?) - I'm looking for a review today. Thanks!

@archmoj
Copy link
Contributor

archmoj commented Jul 22, 2019

Thanks @pynklu & @etpinard.
Nicely done.
💃

@etpinard etpinard merged commit 84e79e4 into plotly:master Jul 22, 2019
@pynklu pynklu deleted the 2206-configurable-double-click-delay branch August 27, 2019 07:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants