Skip to content

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.

Sign up for GitHub

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

Merged
alexcjohnson merged 3 commits into from
Aug 6, 2018
Merged

Pie colors fix & enhancements #2870

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d50334e
pie color fix & enhancements
alexcjohnson Aug 3, 2018
ded6e94
update from calcInteractions to crossTraceCalc
alexcjohnson Aug 4, 2018
0bf82c8
stop exporting doCrossTraceCalc
alexcjohnson Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,6 @@ exports.plot = function(gd, data, layout, config) {
return;
}

Plots.doCrossTraceCalc(gd);

// calc and autorange for errorbars
Registry.getComponentMethod('errorbars', 'calc')(gd);

// TODO: autosize extra for text markers and images
// see https://github.com/plotly/plotly.js/issues/1111
return Lib.syncOrAsync([
Expand Down Expand Up @@ -331,7 +326,6 @@ exports.plot = function(gd, data, layout, config) {
];

if(hasCartesian) seq.push(positionAndAutorange);
else seq.push(Plots.doCrossTraceCalc);

seq.push(subroutines.layoutStyles);
if(hasCartesian) seq.push(drawAxes);
Expand Down
32 changes: 15 additions & 17 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2236,8 +2236,6 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)

plots.supplyDefaults(gd);
plots.doCalcdata(gd);
plots.doCrossTraceCalc(gd);
Registry.getComponentMethod('errorbars', 'calc')(gd);

return Promise.resolve();
}
Expand Down Expand Up @@ -2503,8 +2501,6 @@ plots.doCalcdata = function(gd, traces) {
// clear stuff that should recomputed in 'regular' loop
if(hasCalcTransform) clearAxesCalc(axList);

var calcInteractionsFuncs = [];

function calci(i, isContainer) {
trace = fullData[i];
_module = trace._module;
Expand All @@ -2530,12 +2526,6 @@ plots.doCalcdata = function(gd, traces) {

if(_module && _module.calc) {
cd = _module.calc(gd, trace);

// Some modules need to update traces' calcdata after
// *all* traces have been through calc - so later traces can
// impact earlier traces.
var calcInteractions = _module.calcInteractions;
if(calcInteractions) Lib.pushUnique(calcInteractionsFuncs, calcInteractions);
}
}

Expand All @@ -2561,9 +2551,10 @@ plots.doCalcdata = function(gd, traces) {
for(i = 0; i < fullData.length; i++) calci(i, true);
for(i = 0; i < fullData.length; i++) calci(i, false);

for(i = 0; i < calcInteractionsFuncs.length; i++) calcInteractionsFuncs[i](gd, calcdata);
plots.doCrossTraceCalc(gd);
Copy link
Copy Markdown
Collaborator Author

@alexcjohnson alexcjohnson Aug 4, 2018

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...

Copy link
Copy Markdown
Collaborator Author

@alexcjohnson alexcjohnson Aug 6, 2018

Choose a reason for hiding this comment

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

stopped exporting -> 0bf82c8


Registry.getComponentMethod('fx', 'calc')(gd);
Registry.getComponentMethod('errorbars', 'calc')(gd);
};

function clearAxesCalc(axList) {
Expand Down Expand Up @@ -2599,14 +2590,21 @@ plots.doCrossTraceCalc = function(gd) {
var methods = hash[k];
var subplots = fullLayout._subplots[k];

for(i = 0; i < subplots.length; i++) {
var sp = subplots[i];
var spInfo = k === 'cartesian' ?
fullLayout._plots[sp] :
fullLayout[sp];
if(Array.isArray(subplots)) {
for(i = 0; i < subplots.length; i++) {
var sp = subplots[i];
var spInfo = k === 'cartesian' ?
fullLayout._plots[sp] :
fullLayout[sp];

for(j = 0; j < methods.length; j++) {
methods[j](gd, spInfo);
}
}
}
else {
for(j = 0; j < methods.length; j++) {
methods[j](gd, spInfo);
methods[j](gd);
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion src/traces/pie/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,8 +130,9 @@ exports.calc = function calc(gd, trace) {
* This is done after sorting, so we pick defaults
* in the order slices will be displayed
*/
exports.calcInteractions = function(gd, calcdata) {
exports.crossTraceCalc = function(gd) {
var fullLayout = gd._fullLayout;
var calcdata = gd.calcdata;
var pieColorWay = fullLayout.piecolorway;
var colorMap = fullLayout._piecolormap;

Expand Down
2 changes: 1 addition & 1 deletion src/traces/pie/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Pie.layoutAttributes = require('./layout_attributes');

var calcModule = require('./calc');
Pie.calc = calcModule.calc;
Pie.calcInteractions = calcModule.calcInteractions;
Pie.crossTraceCalc = calcModule.crossTraceCalc;

Pie.plot = require('./plot');
Pie.style = require('./style');
Expand Down
2 changes: 1 addition & 1 deletion test/jasmine/tests/annotations_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ describe('annotations autorange', function() {
});
})
.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]);
Copy link
Copy Markdown
Collaborator Author

@alexcjohnson alexcjohnson Aug 4, 2018

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.

return Plotly.relayout(gd, 'annotations[0].text', 'a');
})
.then(function() {
Expand Down
8 changes: 0 additions & 8 deletions test/jasmine/tests/bar_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1801,14 +1801,6 @@ function mockBarPlot(dataWithoutTraceType, layout) {
supplyAllDefaults(gd);
Plots.doCalcdata(gd);

var plotinfo = {
xaxis: gd._fullLayout.xaxis,
yaxis: gd._fullLayout.yaxis
};

// call Bar.crossTraceCalc
Bar.crossTraceCalc(gd, plotinfo);
Copy link
Copy Markdown
Collaborator Author

@alexcjohnson alexcjohnson Aug 4, 2018

Choose a reason for hiding this comment

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

⚠️ ⚠️ ⚠️ Removed because this is included in 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.

Copy link
Copy Markdown
Contributor

@etpinard etpinard Aug 6, 2018

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.


return gd;
}

Expand Down