Cartesian subplot fixes #1049
Cartesian subplot fixes #1049
Conversation
- so that old axis scale fields (e.g _length, _m, ...) in fullLayout don't get relinked in subplot objects, leading to incorrect relayout / resize.
- to make sure that subplot nodes have the correct mouse / click handlers.
|
@rreusser would mind taking a 👁️ on this? |
| @@ -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); | |||
There was a problem hiding this comment.
Ah, so it was that the axes were outdated? That seems consistent with what was observed.
| @@ -630,6 +631,16 @@ describe('hover after resizing', function() { | |||
|
|
|||
| afterEach(destroyGraphDiv); | |||
|
|
|||
| function _click(pos) { | |||
There was a problem hiding this comment.
👍 for fixing via testing!
| @@ -248,7 +248,7 @@ describe('Test Plots', function() { | |||
| describe('Plots.resize', function() { | |||
| var gd; | |||
|
|
|||
| beforeEach(function(done) { | |||
| beforeAll(function(done) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| expect(fullLayout.yaxis._length).toEqual(220); | ||
|
|
||
| expect(plotinfo.xaxis._length).toEqual(240); | ||
| expect(plotinfo.yaxis._length).toEqual(220); |
There was a problem hiding this comment.
Got it. Sounds good! I once misunderstood beforeAll to mean before each all tests, so thought it was worth checking!
Agreed. I would be nice to beef up our resize test suites. Let's first publish Thanks very much for reviewing this! |
|
💃 |
fixes #1044
and fixes relayout problems discovered in http://community.plot.ly/t/resizing-is-not-working-after-setting-height-in-plotly-relayout/2196/6?u=etienne
These problems were introduced in #946
This PR will need to merge
masterafter #1048 to ✅