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

Selection on `choropleth` to work even if an invisible trace is present #2099

Merged
merged 5 commits into from Oct 18, 2017

Conversation

Projects
None yet
3 participants
@monfera
Copy link
Contributor

commented Oct 18, 2017

Box select and lasso to work in the presence of an invisible choropleth trace. Updated test case signals if the small fix is commented out.

@alexcjohnson
Copy link
Contributor

left a comment

Seems reasonable - but I wonder if we should preempt problems like this by just filtering out invisible traces (visible !== true, to account for legendonly) here?

@monfera

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

Thanks @alexcjohnson, I understand the general direction for preventing the situation from emerging, not sure how I'd go about it as node3 etc. are handled outside the scope of selection. I'd need to urgently work on table.restyle but circle back to this ASAP.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Oct 18, 2017

@monfera the idea is that the only reason node3 would not be set is if the trace is not visible. So if we filter invisible traces out before even calling selectPoints, we don't have to worry about this kind of error in any of the individual modules.

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

but I wonder if we should preempt problems like this by just filtering out invisible traces (visible !== true, to account for legendonly)

This is a much better solution in my opinion.

@@ -190,6 +195,7 @@ module.exports = function prepSelect(e, startX, startY, dragOptions, mode) {
selection = [];
for(i = 0; i < searchTraces.length; i++) {
searchInfo = searchTraces[i];
if(!visible(searchInfo)) continue;

This comment has been minimized.

Copy link
@etpinard

etpinard Oct 18, 2017

Member

Nice. Does that mean we can 🔪 those visible !== false clauses here, here, here, here and here?

This comment has been minimized.

Copy link
@monfera

monfera Oct 18, 2017

Author Contributor

Great find, indeed these are the only callers, and select check is almost always on the top, except scattermapbox/select/js where it's okay to set the _hasDimmedPts to false anyway:

    trace._hasDimmedPts = false;

    if(!subtypes.hasMarkers(trace)) return [];

Pushing a new commit now

@monfera

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

moved check more upstream on Alex's suggestion

@monfera monfera force-pushed the invisible-select branch from e757114 to fc0ad56 Oct 18, 2017

@etpinard

This comment has been minimized.

Copy link
Member

commented Oct 18, 2017

💃 for me. Great PR @monfera !

@monfera

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

Thanks @etpinard and @alexcjohnson for the added clarity and code size reducing suggestions!

@monfera monfera merged commit fff9c6e into master Oct 18, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@monfera monfera deleted the invisible-select branch Oct 18, 2017

etpinard added a commit that referenced this pull request Oct 20, 2017

fixup choropleth select test
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.

etpinard added a commit that referenced this pull request Oct 23, 2017

fixup choropleth select test
- PR #2099 got merged
  on a branch behind #2081
  which caused the test to fail on master.

@etpinard etpinard referenced this pull request Oct 23, 2017

Merged

Branch for 1.31.2 #2111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.