Expose minreducedwidth and minreducedheight to API#6307
Conversation
|
@hannahker Thanks very much for the PR 🏅 Let's also add a jasmine test at the end of describe('Test Plots automargin extra', function() {
var gd;
beforeEach(function() {
gd = createGraphDiv();
});
afterEach(destroyGraphDiv);
it('should resize the plot area when twaeking min-reduced width & height', function(done) {
function assert(attr, exp) {
var xy = d3Select('rect.nsewdrag')[0][0];
expect(xy.getAttribute(attr)).toEqual(exp);
}
var fig = require('@mocks/z-automargin-minreducedheight.json');
Plotly.newPlot(gd, fig)
.then(function() {
assert('width', '70');
assert('height', '55');
})
.then(function() {
return Plotly.relayout(gd, 'margin.minreducedheight', 100);
})
.then(function() {
assert('width', '70');
assert('height', '100');
})
.then(function() {
return Plotly.relayout(gd, 'margin.minreducedwidth', 100);
})
.then(function() {
assert('width', '100');
assert('height', '100');
})
.then(done, done.fail);
});
}); |
|
|
||
| Plotly.newPlot(gd, fig) | ||
| .then(function() { | ||
| assert('height', '55'); |
There was a problem hiding this comment.
Thanks @archmoj for suggesting this test. Copied it over here but just dropped a couple of the width assertions as I noticed that this was actually a bit variable when margin.minreducedwidth wasn't specified. For example, I checked between a couple browsers and saw plot widths of both 64px and 71px initially. Do you know why this would be?
There was a problem hiding this comment.
I bet it is related to each OS+browser rendering fonts slightly differently.
FYI - We mainly use Chrome on CircleCI; but we also have tests using Firefox.
You may consider testing the initial width values to be in a range thanks to Jasmine's toBeCloseTo.
| dflt: 64, | ||
| editType: 'plot', | ||
| description: 'Minimum height of the plot with automargin applied (in px)' | ||
| }, |
There was a problem hiding this comment.
@alexcjohnson Is there a better place to put these new attributes in?
It looks like these are related to automargin not margin itself.
There was a problem hiding this comment.
@archmoj and I discussed this a bit earlier today, and I think the conclusion we came to was: because margin.autoexpand controls whether or not these attributes are even relevant, (1) they should stay here, so they're in the same container as autoexpand, and (2) if autoexpand is false we shouldn't even coerce these two new ones. (The general rule is, if an attribute doesn't do anything because of the value of some other attribute(s), it should not be coerced)
The only question is whether it's really true that these attributes are irrelevant when autoexpand=false. According to its description this would disable automargin from "Legends, colorbars, updatemenus, sliders, axis rangeselector and rangeslider" but what if xaxis.automargin=true? Maybe then the margins can still be pushed even if margin.autoexpand=false? That needs to be tested before we make coercion conditional on margin.autoexpand - but even if that's the case I think it makes sense to leave these attributes where they are.
There was a problem hiding this comment.
yes, @archmoj and I looked into the source code and decided that these attributes would be best placed at the top level of layout parameters as they could be related to autoexpand as well (and I think logically make sense alongside height and width).
|
@archmoj all tests are passing now except the Do you know what might be going on here? I've had these tests all pass in a previous run (and haven't updated the code since). |
Fix variable references Fix formatting
|
@hannahker as one final note, please also provide |
…r increasing control over automargin (plotly/plotly.js#6307), Fix unused attributes in Layout.init and Chart.withLayoutStyle
3 participants
This PR allows these previously global variables to be controlled by the developer, if desired. If specified, these variables will set the minimum height and width of a plot who's size is impacted by
automargin.