-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Return empty set for invisible bar trace #2081
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
Good catch. I'd vote for making: if(selection.length) { /* */
// turn into
if(selection && selection.length) { /* */ that way we won't have to remember to always return an array in all the trace module |
@etpinard it's not Also, we can't just do
We probably don't want Looking at this block a bit more, it feels like it could be simplified: As resolution of this |
Ok, if you think that's more elegant, I won't argue with you. Just make sure to lock that down in tests for all |
Good catch @monfera - I don't care exactly what route you take, but as long as the event always has a |
|
||
// to make sure none of the above tests fail with extraneous invisible traces, | ||
// add a bunch of them here | ||
function addInvisible(fig, canHaveLegend) { |
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.
Brilliant ✨
💃 nice tag-team effort on this one |
@alexcjohnson incredible commit, thanks! |
Downstream processing expects an array here, even when a trace is not
visible
:(side note:
fillSelectionItem
handles the case of a non-arrayselection
, in which case it just passes it on ie. doesn't try to setcurveNumber
etc. properties onundefined
; the result of which is the sameundefined
as the value of the first arg)There's a similar
return;
inscatter
,scattergeo
,scattermapbox
.Is it a good direction @etpinard ?