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

1852 persistent click via select points #2944

Merged
merged 56 commits into from
Sep 10, 2018

Conversation

rmoestl
Copy link
Contributor

@rmoestl rmoestl commented Aug 27, 2018

Superseding PR #2824, this PR will introduce the select-on-click / persistent selection feature discussed in issue #1852.

A few remarks:

  • There's one uncertainty regarding testing interactions that should have no effect but might be processed asynchronously. See open TODOs in select_test.js.
  • For now I've decided to not clear selection when user clicks a blank spot. I felt, that this cause losing a selection to easily. Instead clear a selection by click is easy with double click in lasso / select mode, and in zoom / pan mode one only needs to select one point and click it again.
  • As per the original request in Persistent and Multiple Clicking and Unclicking Behaviour #1852 we've not decided how the user "would be informed that points are clickable".

Reason: to be reviewable from a perf standpoint.
- Set hoverMode to 'closest' where needed.
- Instead of an expandingIndex to identify the actual trace
  wanted, expecting a searchInfo object.
- Reason: the selection cache wasn't cleared when a user clicked the
  last selected point with Shift and thus a new selection started with
  Shift would add to the still cached selection testers from previous
  selection.
- Ensure click-to-select cleanly clears and starts selections
  although add/subtract mode on.
- Ensure double click in lasso and select mode cleanly clears
  and restarts selections when add/subtract mode on.
- TODO: deactivated a mouseout listener in geo.js cause
  with it, click-to-select won't work. To be reviewed.
- Include additional for choropleth that's based on geo.
- One double click location in an existing choropleth test
  needed to change though to be on an empty area that
  is not a world country.
- Reason: some tests ran into async timeout issues on CI.
- Thereby fixed bug in geo base plot: set a subplot id in dragOptions
  so that selection cache isn't coerced each time an interaction
  happens.
- Ternary was the only base plot with inconsistent behavior in terms
  of holding the Shift key. It allowed to temporarily pan (zoom when
  in pan) in zoom, lasso and select drag modes.
- Updates of TODO comments are based on discussions on Github
  and Slack.
- Reason: isPointOrBinSelected is a potentially costly operation
  and thus leverage an assignment in an if condition trick
  to avoid executing isPointOrBinSelected unnecessarily.
- Renaming because since click-to-select, selections not only
  can be defined by polygons, but also by point numbers.
  Up until now code was centered around polygons being
  the sole means to define a selection. Renamed
  identifiers to be more general.
- Extract `multitester` to new module `lib/select.js` because
  `multitester` is no longer to been seen in the
  sole context of polygons.
@etpinard
Copy link
Contributor

I suggest to consider this a separate issue (if not already known).

Thanks for the report -> #2958

@rmoestl
Copy link
Contributor Author

rmoestl commented Aug 30, 2018

@etpinard, regarding this bug

Now, I noticed one potential bug (I can't remember if this is something we agreed on earlier):

  • start from mock 12.json (but I think all selectable mocks behaves this way)
  • call relayout('clickmode', 'select')
  • call relayout('dragmode', 'select')
  • select something
  • call relayout('dragmode', 'pan') // or 'zoom'
  • shift-click-select a pt
  • this clear all selected pts

This is a little unfortunate. It all happens when the selection state is cleared in dragbox, see

clearAndResetSelect();

I believe we do that here because if we've drawn polygons previously in lasso / select and then we pan or zoom the plotting area, these polygons are no longer valid. Why? Because panning / zooming isn't implemented for those polygons. Haven't looked at the code explicitly, but that's my interpretation when testing it interactively.

If my interpretation is correct, what could we do?

  1. Zoom and pan polygons accordingly (superb UX-wise)
  2. Defer clearing selection state until pan an zoom really happens (i.e. calling clearAndResetSelect not in dragElement.prepFn but in first call of dragElement.moveFn. Not perfect of course. A user would still expect that selection is retained although he zoomed / panned.

@etpinard
Copy link
Contributor

@rmoestl about #2944 (comment). First, thanks very much for the detailed response.

Thinking about this again, I'm starting to believe that the current behavior isn't that bad. Or, in other words, fixing it won't be worth it. The current behavior only looks odd when users shift-select pt in zoom or pan mode before actually zoom or panning. Clearing selection state during prepFn is what we want until find a solution to your (1) pt, but that's for another PR. Consider my comment as non-blocking.

@@ -16,6 +16,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
return Lib.coerce(layoutIn, layoutOut, layoutAttributes, attr, dflt);
}

coerce('clickmode');
Copy link
Contributor

@etpinard etpinard Sep 6, 2018

Choose a reason for hiding this comment

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

We're still missing @alexcjohnson's smart default suggestion where when a user sets clickmode: 'select' the default hovermode value becomes 'closest'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, sorry, I misunderstood that. I thought mentioning in clickmode's description that hovermode set to closest is best for most applications was "enough".

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So what we want to have is when clickmode includes the select flag and nothing is set explicitly for hovermode, the default of hovermode should be closest. Is that right?

Yep! Which should be as simple as modifying the hovermodeDflt logic below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

src/lib/polygon.js Outdated Show resolved Hide resolved
- Reason: it's pretty sure it'll no longer be called.
- See #2944 (comment)
@etpinard
Copy link
Contributor

Amazing work @rmoestl . A hard-fought 💃

This thing will be in the running for PR of the year 🏆

@rmoestl
Copy link
Contributor Author

rmoestl commented Sep 10, 2018

🙇‍♂️ Thanks for your support, @etpinard and @alexcjohnson!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants