Skip to content

Commit a3c70f8

Browse files
committed
fix(sankey): derive effective node.pad clamp from layout geometry
The node.pad warning read the clamped padding back through sankey.nodePadding(). In @plotly/d3-sankey@0.7.x that getter returns the post-layout clamped value, but since 0.12.x (upstream split of dy/py) it returns the configured value, so the comparison is never true and the warning never fires after a dependency upgrade. Measure the smallest vertical gap between consecutive nodes in any one column instead; this works regardless of the installed d3-sankey version. Regression tests cover both the clamped and non-clamped paths. Refs #7832
1 parent 95bfea1 commit a3c70f8

2 files changed

Lines changed: 80 additions & 2 deletions

File tree

src/traces/sankey/render.js

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -73,8 +73,30 @@ function sankeyModel(layout, d, traceIndex) {
7373

7474
var graph = sankey();
7575

76-
if(sankey.nodePadding() < nodePad) {
77-
Lib.warn('node.pad was reduced to ', sankey.nodePadding(), ' to fit within the figure.');
76+
// Derive the effective (post-clamp) node padding from the laid-out node
77+
// geometry instead of reading it back through `sankey.nodePadding()`.
78+
// In @plotly/d3-sankey@0.7.x that getter returned the clamped value after
79+
// the layout ran, but since 0.12.x it returns the user-configured value
80+
// (upstream split `dy` from `py`), so a getter-based check would never
81+
// fire. Measuring the smallest vertical gap between consecutive nodes in
82+
// any one column is version-independent. See #7832.
83+
var effectivePad = nodePad;
84+
var columns = {};
85+
graph.nodes.forEach(function(node) {
86+
var col = Math.round(node.x0);
87+
if(!columns[col]) columns[col] = [];
88+
columns[col].push([node.y0, node.y1]);
89+
});
90+
for(var key in columns) {
91+
var column = columns[key].sort(function(a, b) { return a[0] - b[0]; });
92+
for(i = 1; i < column.length; i++) {
93+
var gap = column[i][0] - column[i - 1][1];
94+
if(gap < effectivePad) effectivePad = gap;
95+
}
96+
}
97+
98+
if(effectivePad < nodePad) {
99+
Lib.warn('node.pad was reduced to ', effectivePad, ' to fit within the figure.');
78100
}
79101

80102
// Counters for nested loops

test/jasmine/tests/sankey_test.js

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,62 @@ describe('sankey tests', function() {
9595
});
9696
});
9797

98+
describe('node.pad reduction warning', function() {
99+
// The warning must be driven by the effective (post-clamp) padding,
100+
// not by reading `sankey.nodePadding()` back, which since
101+
// @plotly/d3-sankey@0.12.x returns the configured value instead of
102+
// the clamped one - see #7832.
103+
var padMock = [{
104+
type: 'sankey',
105+
layoutversion: 2,
106+
domain: {x: [0, 1], y: [0, 1]},
107+
node: {
108+
label: Array.from({length: 24}, function(_, i) { return 'n' + i; }),
109+
pad: 30,
110+
thickness: 10
111+
},
112+
link: {
113+
source: Array.from({length: 23}, function(_, i) { return i; }),
114+
target: Array.from({length: 23}, function(_, i) { return i + 1; }),
115+
value: Array.from({length: 23}, function() { return 1; })
116+
}
117+
}];
118+
119+
it('warns when the figure is too small for node.pad', function(done) {
120+
var warnings = [];
121+
spyOn(Lib, 'warn').and.callFake(function(msg) {
122+
warnings.push(msg);
123+
});
124+
125+
var gd = createGraphDiv('pad-warn-small', 300, 100);
126+
Plotly.newPlot(gd, Lib.extendDeep([], padMock))
127+
.then(function() {
128+
expect(warnings.length).toEqual(1);
129+
expect(warnings[0][0]).toBe('node.pad was reduced to ');
130+
expect(warnings[0][1]).toBeLessThan(30);
131+
return Plotly.purge(gd);
132+
})
133+
.then(function() { destroyGraphDiv(gd); })
134+
.then(done, done.fail);
135+
});
136+
137+
it('does not warn when the figure fits node.pad', function(done) {
138+
var warnings = [];
139+
spyOn(Lib, 'warn').and.callFake(function(msg) {
140+
warnings.push(msg);
141+
});
142+
143+
var gd = createGraphDiv('pad-warn-large', 700, 900);
144+
Plotly.newPlot(gd, Lib.extendDeep([], padMock))
145+
.then(function() {
146+
expect(warnings.length).toEqual(0);
147+
return Plotly.purge(gd);
148+
})
149+
.then(function() { destroyGraphDiv(gd); })
150+
.then(done, done.fail);
151+
});
152+
});
153+
98154
describe('sankey global defaults', function() {
99155
it('should not coerce trace opacity', function() {
100156
var gd = Lib.extendDeep({}, mock);

0 commit comments

Comments
 (0)