-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix scattergl selectedpoints clearance under select/lasso drag modes #2492
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
Conversation
- no need for two separate viewport computation blocks - break up one more loooooooooong line
@dfcreative can you review this PR please? |
All right - merging this. |
var symbolNoDot = !!Drawing.symbolNoDot[symbolNumber % 100]; | ||
var symbolNoFill = !!Drawing.symbolNoFill[symbolNumber % 100]; | ||
|
||
var isDot = constants.DOT_RE.test(symbol); |
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.
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.
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.
Good eye. This probably deserves an issue of its own.
|
||
if(subTypes.hasMarkers(trace)) { | ||
opts.marker = convertMarkerStyle(trace); | ||
opts.selected = convertMarkerSelection(trace, trace.selected); |
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.
In some reason trace.selected
is empty here for multiple colors/multiple opacities #2500
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.
Ok - I'll take a look at this tomorrow
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