-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix stacked area layering and deletion bugs #3005
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
…scatter traces
and along the way speed up trace deletion in general, by not redrawing all the points in the deleted trace immediately before removing them!
src/traces/scatter/plot.js
Outdated
@@ -72,7 +72,7 @@ module.exports = function plot(gd, plotinfo, cdscatter, scatterLayer, transition | |||
}); | |||
}); | |||
} else { | |||
scatterLayer.selectAll('g.trace').each(function(d, i) { | |||
join.each(function(d, i) { |
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.
@etpinard it doesn't seem like animations can remove traces, is that correct? That's what it looks like, since I don't see a join.exit()
associated with them (since isFullReplot = !transitionOpts
)... yet the comment above implies that you can add traces:
// Must run the selection again since otherwise enters/updates get grouped together
// and these get executed out of order. Except we need them in order!
So... do I need to worry about animations deleting traces or not?
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.
do I need to worry about animations deleting traces or not?
Right, removing traces with 'frame.redraw': false
(i.e. without a replot at the end of the animation), doesn't work currently:
https://codepen.io/etpinard/pen/ZMRReB
so I don't think you broke anything here.
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.
Ah Ricky wrote an issue about this #932
@etpinard 3rd time's a charm, but the intermittent error in |
Nice solution! Would you mind adding a few words about this behavior in the |
Good call, I added it to both |
nicely done 💃 |
Lumped fixes for #3002 and #3003 into one PR, since they touch the same code.
stackgroup
. The "next" trace must be in the same stack group, or no stack group. Second, once you've done this, it doesn't make much sense to preserve the trace ordering as it is ingd.data
exactly. Traces in the same stack group, or that fill to each other, need to be in consecutive order - or you can get weird effects like Issue with fill in stackgroups #3003 (comment). As mentioned there, I chose to push traces only backward to meet the first in their stack, but otherwise to preserve the existing order as much as possible. The new mock demonstrates this.