-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Pie colors fix & enhancements #2870
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
- fix inheritance of explicit colors by later traces - allow inheritance (of explicit colors) by earlier traces too - add `piecolorway` and `extendpiecolors` for more control over pie colors
and simplify by moving more stuff into doCalcdata
src/plots/plots.js
Outdated
| for(i = 0; i < fullData.length; i++) calci(i, false); | ||
|
|
||
| for(i = 0; i < calcInteractionsFuncs.length; i++) calcInteractionsFuncs[i](gd, calcdata); | ||
| plots.doCrossTraceCalc(gd); |
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.
Hmm, I guess we don't need to export doCrossTraceCalc anymore since it's only used 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.
stopped exporting -> 0bf82c8
| }) | ||
| .then(function() { | ||
| _assert('auto rng / big tx', [-0.22, 3.57], [0.84, 3.365]); | ||
| _assert('auto rng / big tx', [-0.22, 3.59], [0.84, 3.365]); |
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.
Doesn't matter for CI, and has nothing to do with this PR, but this change makes this test pass on my Mac.
| }; | ||
|
|
||
| // call Bar.crossTraceCalc | ||
| Bar.crossTraceCalc(gd, plotinfo); |
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.
doCalcdata now, but in fact the test FAILS if we leave this in. That means Bar.crossTraceCalc is not idempotent, though I didn't really look into what it's doing. I don't think this is particularly a problem, but it might indicate it's more fragile than it should be.
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.
That means Bar.crossTraceCalc is not idempotent, though I didn't really look into what it's doing.
Yep, I noticed that last week. This commit dfada6a helps, but there are more cases where Bar.setPositions oops I mean Bar.crossTraceCalc mutates gd.calcdata.
| .then(done); | ||
| }); | ||
|
|
||
| it('can use a separate pie colorway and disable extended colors', function(done) { |
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.
Great tests!
|
Very nice. Merging the old As an aside, I think it might be best at some point to move the 💃 |
Excellent idea. In general it needs to be in |
Reboot of #2867
Original commit d50334e:
calcInteractionsstep immediately aftercalc.layout.piecolorwayandlayout.extendpiecolorsfor more control over pie colors (cc @nicolaskruchten @VeraZab)Update after #2868, commit ded6e94:
calcInteractionsand usescrossTraceCalcfor piescrossTraceCalcto work with pie (and other subplot-less traces)doCalcdata. In particular, it doesn't need to happen in any other places besidesdoCalcdata, andErrorbars.calccan move in here too since it just needed to happen aftercrossTraceCalc.