Skip to content
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

Automargin zoom #6312

Merged
merged 12 commits into from Sep 15, 2022
1 change: 1 addition & 0 deletions draftlogs/6312_fix.md
@@ -0,0 +1 @@
Fix bug with automargin and axis titles [[#6312](https://github.com/plotly/plotly.js/pull/6312)]
2 changes: 0 additions & 2 deletions src/plot_api/plot_api.js
Expand Up @@ -1888,8 +1888,6 @@ function addAxRangeSequence(seq, rangesAltered) {
}
}
}

if(ax.automargin) skipTitle = false;
}

return Axes.draw(gd, axIds, {skipTitle: skipTitle});
Expand Down
Binary file added test/image/baselines/z-automargin-zoom.png
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
60 changes: 60 additions & 0 deletions test/image/mocks/z-automargin-zoom.json
@@ -0,0 +1,60 @@
{"data": [{
"showlegend": false,
"type": "scatter",
"x": [
"@100000001",
"@100000002",
"@100000003",
"@100000004",
"@100000005",
"@100000006",
"@100000007",
"@100000008",
"@100000009",
"@100000010",
"@100000011",
"@100000012",
"@100000013",
"@100000014",
"@100000015",
"@100000016",
"@100000017",
"@100000018",
"@100000019"
],
"xaxis": "x",
"y": [
100,
101,
99,
101,
98,
97,
101,
102,
109,
110,
110,
102,
93,
114,
115,
115,
171,
185,
190
],
"yaxis": "y"
}],
"layout": {
"height": 400,
"width": 700,
"xaxis": {
"title": {"text": "Revision"},
"automargin": true
},
"yaxis": {
"automargin": true
}
}
}
27 changes: 27 additions & 0 deletions test/jasmine/tests/axes_test.js
Expand Up @@ -4376,6 +4376,33 @@ describe('Test axes', function() {
})
.then(done, done.fail);
});
it('should respect axis title placement on relayout', function(done) {
function getPos(gd, sel) {
return d3Select(gd).select(sel).node().getBoundingClientRect();
}

// Tick position is < title position since 0 is at the top of the graph,
// rather than at the bottom. We're checking that the ticks and title don't overlap
function assertLayout() {
var titleTop = getPos(gd, '.xtitle').top;
var tickBottom = getPos(gd, '.xtick').bottom;
expect(tickBottom).toBeLessThan(titleTop + 2); // allow two pixels tolerance
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to note: this specific test looks fine, but only because:
(1) all the tick labels are essentially the same length. select(...).node() gives you only the first element, but the first one is unlikely to conflict with the axis title. Only the labels that are directly above the title can push it down.
(2) the tick labels are dense enough that even when you zoom in the title never fits between them. If that's not the case, when the title lines up right it can actually squeeze up between those labels.

}

var fig = require('@mocks/z-automargin-zoom.json');
Plotly.newPlot(gd, fig)

.then(function() {
assertLayout();
})
archmoj marked this conversation as resolved.
Show resolved Hide resolved
.then(function() {
return Plotly.relayout(gd, {'xaxis.range': [6, 14]});
})
.then(function() {
assertLayout();
})
.then(done, done.fail);
});
});

describe('zeroline visibility logic', function() {
Expand Down