Skip to content

Cartesian subplot fixes #1049

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
merged 4 commits into from
Oct 18, 2016
Merged

Cartesian subplot fixes #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f4ca110
plots: relink subplot before relink _ keys in fullLayout
etpinard Oct 17, 2016
2c04994
lint: space out .then clauses
etpinard Oct 17, 2016
d61b873
call Fx.init in after marginPushers step
etpinard Oct 17, 2016
57cc9ca
Merge branch 'master' into cartesian-relayout-fix
etpinard Oct 17, 2016
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: 5 additions & 1 deletion src/plot_api/plot_api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,12 +196,16 @@ Plotly.plot = function(gd, data, layout, config) {
return Plots.previousPromises(gd);
}

// in case the margins changed, draw margin pushers again
function marginPushersAgain() {
// in case the margins changed, draw margin pushers again
var seq = JSON.stringify(fullLayout._size) === oldmargins ?
[] :
[marginPushers, subroutines.layoutStyles];

// re-initialize cartesian interaction,
// which are sometimes cleared during marginPushers
seq = seq.concat(Fx.init);

return Lib.syncOrAsync(seq, gd);
}

Expand Down
6 changes: 3 additions & 3 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -402,6 +402,9 @@ plots.supplyDefaults = function(gd) {
// clean subplots and other artifacts from previous plot calls
plots.cleanPlot(newFullData, newFullLayout, oldFullData, oldFullLayout);

// relink / initialize subplot axis objects
plots.linkSubplots(newFullData, newFullLayout, oldFullData, oldFullLayout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, so it was that the axes were outdated? That seems consistent with what was observed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly.


// relink functions and _ attributes to promote consistency between plots
relinkPrivateKeys(newFullLayout, oldFullLayout);

Expand All @@ -416,9 +419,6 @@ plots.supplyDefaults = function(gd) {
ax.setScale();
}

// relink / initialize subplot axis objects
plots.linkSubplots(newFullData, newFullLayout, oldFullData, oldFullLayout);

// update object references in calcdata
if((gd.calcdata || []).length === newFullData.length) {
for(i = 0; i < newFullData.length; i++) {
Expand Down
41 changes: 33 additions & 8 deletions test/jasmine/tests/hover_label_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var Lib = require('@src/lib');
var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var mouseEvent = require('../assets/mouse_event');
var click = require('../assets/click');
var doubleClick = require('../assets/double_click');

describe('hover info', function() {
Expand Down Expand Up @@ -630,6 +631,16 @@ describe('hover after resizing', function() {

afterEach(destroyGraphDiv);

function _click(pos) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 for fixing via testing!

return new Promise(function(resolve) {
click(pos[0], pos[1]);

setTimeout(function() {
resolve();
}, constants.HOVERMINTIME);
});
}

function assertLabelCount(pos, cnt, msg) {
return new Promise(function(resolve) {
mouseEvent('mousemove', pos[0], pos[1]);
Expand All @@ -652,22 +663,36 @@ describe('hover after resizing', function() {
pos1 = [401, 122];

Plotly.plot(gd, data, layout).then(function() {

// to test https://github.com/plotly/plotly.js/issues/1044

return _click(pos0);
})
.then(function() {
return assertLabelCount(pos0, 1, 'before resize, showing pt label');
}).then(function() {
})
.then(function() {
return assertLabelCount(pos1, 0, 'before resize, not showing blank spot');
}).then(function() {
})
.then(function() {
return Plotly.relayout(gd, 'width', 500);
}).then(function() {
})
.then(function() {
return assertLabelCount(pos0, 0, 'after resize, not showing blank spot');
}).then(function() {
})
.then(function() {
return assertLabelCount(pos1, 1, 'after resize, showing pt label');
}).then(function() {
})
.then(function() {
return Plotly.relayout(gd, 'width', 600);
}).then(function() {
})
.then(function() {
return assertLabelCount(pos0, 1, 'back to initial, showing pt label');
}).then(function() {
})
.then(function() {
return assertLabelCount(pos1, 0, 'back to initial, not showing blank spot');
}).then(done);
})
.then(done);
});
});

Expand Down
13 changes: 12 additions & 1 deletion test/jasmine/tests/plots_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ describe('Test Plots', function() {
describe('Plots.resize', function() {
var gd;

beforeEach(function(done) {
beforeAll(function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm… beforeAll? Usually when I've written that, it's not actually what I want. Does it leak state between the tests? Is it definitely what you meant here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, this is on purpose.

This particular describe creates a graph then calls Plotly.Plots.resize. The multiple it block are simply there to group the assertion in some logical way. There's no need to re-create the graph before each it in this case.

gd = createGraphDiv();

Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
Expand Down Expand Up @@ -287,6 +287,17 @@ describe('Test Plots', function() {
expect(svgHeight).toBe(400);
}
});

it('should update the axis scales', function() {
var fullLayout = gd._fullLayout,
plotinfo = fullLayout._plots.xy;

expect(fullLayout.xaxis._length).toEqual(240);
expect(fullLayout.yaxis._length).toEqual(220);

expect(plotinfo.xaxis._length).toEqual(240);
expect(plotinfo.yaxis._length).toEqual(220);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In particular @rreusser , these assertions were failing before f4ca110

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it. Sounds good! I once misunderstood beforeAll to mean before each all tests, so thought it was worth checking!

});
});

describe('Plots.purge', function() {
Expand Down