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

Fix scattergl selectedpoints clearance under select/lasso drag modes #2492

Merged
merged 3 commits into from
Mar 26, 2018

Conversation

etpinard
Copy link
Contributor

fixes a bug brought up by @cpsievert in #2298 (comment) - though this PR does not fix the entire issue.

I decided also to push two DRY-ing commits 🌴 , one of which we'll be used during the splom (see #2372) push.

cc @dfcreative

@etpinard
Copy link
Contributor Author

@dfcreative can you review this PR please?

@etpinard
Copy link
Contributor Author

All right - merging this.

@etpinard etpinard merged commit 7316d98 into master Mar 26, 2018
@etpinard etpinard deleted the scattergl-dry-convert branch March 26, 2018 19:41
var symbolNoDot = !!Drawing.symbolNoDot[symbolNumber % 100];
var symbolNoFill = !!Drawing.symbolNoFill[symbolNumber % 100];

var isDot = constants.DOT_RE.test(symbol);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know this isn't new here, just noticing it: this won't work if symbol is provided by number in the first place. Better to just test the number once we have it - ie isDot = symbolNumber >= 200 (and for isOpen isOpen = (symbolNumber % 200) >= 100 - perhaps we should make these into helper functions in the Drawing module?). Probably not a very well-known feature, but it is supported by SVG, and could be useful particularly for symbol arrays, as you could use a typed array.

Dunno if anyone would use both names and numbers in the same plot, but if they did, it would also help to key the SYMBOL_SDF cache off the number.

Copy link
Contributor Author

@etpinard etpinard Mar 26, 2018

Choose a reason for hiding this comment

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

Good eye. This probably deserves an issue of its own.


if(subTypes.hasMarkers(trace)) {
opts.marker = convertMarkerStyle(trace);
opts.selected = convertMarkerSelection(trace, trace.selected);
Copy link
Contributor

Choose a reason for hiding this comment

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

In some reason trace.selected is empty here for multiple colors/multiple opacities #2500

Copy link
Contributor Author

@etpinard etpinard Mar 26, 2018

Choose a reason for hiding this comment

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

Ok - I'll take a look at this tomorrow

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

Successfully merging this pull request may close these issues.

3 participants