Skip to content

Automargin fixes #2681

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 17 commits into from
Jun 6, 2018
Merged

Automargin fixes #2681

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
970db80
stop mutating user data to add uids
alexcjohnson May 29, 2018
26278de
some test cleanup
alexcjohnson May 29, 2018
6dd68bd
first cut at updating automargin logic
alexcjohnson May 30, 2018
fa86147
refactor colorbars for simpler connection to trace attributes
alexcjohnson May 30, 2018
95f9b33
test legend, rangeselector, rangeslider automargin updates
alexcjohnson May 30, 2018
8f3da82
provide uids to traces a little earlier - to fix carpet clip ids
alexcjohnson May 30, 2018
f9fbbc6
:hocho: colorbar.vals - seems like that was all obsolete anyway
alexcjohnson May 30, 2018
222f396
cleanup in sliders_test
alexcjohnson May 31, 2018
bd07df7
simplify inner plot size tests with custom_assertions
alexcjohnson Jun 1, 2018
84a5695
fix and test slider x positioning when they push margins
alexcjohnson Jun 1, 2018
3deab4c
move sliders to the updated automargin framework
alexcjohnson Jun 1, 2018
bb6c720
use bg rect instead of grids for assertPlotSize
alexcjohnson Jun 1, 2018
d02c76b
base fractional colorbar size on post-expanded-margin plot size
alexcjohnson Jun 2, 2018
5d31324
comment and fix on assertPlotSize with margin.pad
alexcjohnson Jun 6, 2018
16ea22a
reduce automargin padding for small explicit margins
alexcjohnson Jun 6, 2018
24aeb7c
axRange edits don't need drawMarginPushers after all
alexcjohnson Jun 6, 2018
230a782
automargin comment updates
alexcjohnson Jun 6, 2018
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
26 changes: 26 additions & 0 deletions test/jasmine/assets/custom_assertions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,3 +216,29 @@ exports.assertElemInside = function(elem, container, msg) {
contBB.top < elemBB.top &&
contBB.bottom > elemBB.bottom).toBe(true, msg);
};

/*
* quick plot area dimension check: test width and/or height of the inner
* plot area (single subplot) to verify that the margins are as expected
* opts can have keys (all optional):
* width (exact width match)
* height (exact height match)
* widthLessThan (width must be less than this)
* heightLessThan (height must be less than this)
*/
exports.assertPlotSize = function(opts, msg) {
var width = opts.width;
var height = opts.height;
var widthLessThan = opts.widthLessThan;
var heightLessThan = opts.heightLessThan;

var actualWidth = d3.select('.ygrid').node().getBoundingClientRect().width;
var actualHeight = d3.select('.xgrid').node().getBoundingClientRect().height;
Copy link
Contributor

Choose a reason for hiding this comment

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

(non-blocking) but wouldn't d3.select('.subplot.xy > .plot').node().getBoundingClientRect be more general (e.g. would work even on graphs with showgrid: false)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm, good point, but .plot shrinks to the data, which may not reach the edges of the plot area (or could even go beyond with cliponaxis: false). Possibly .bglayer .bg though...

Copy link
Contributor

@etpinard etpinard Jun 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

or could even go beyond with cliponaxis: false

Yeah, good point here. Tough problem it turns out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

switched to .bglayer .bg in bb6c720 though this has a caveat about margin.pad 5d31324. I still think it's an improvement: margin.pad isn't used very much and can be corrected for if you really want, and the bg rect is always present.


var msgPlus = msg ? ': ' + msg : '';

if(width) expect(actualWidth).toBeWithin(width, 1, 'width' + msgPlus);
if(height) expect(actualHeight).toBeWithin(height, 1, 'height' + msgPlus);
if(widthLessThan) expect(actualWidth).toBeLessThan(widthLessThan - 1, 'widthLessThan' + msgPlus);
if(heightLessThan) expect(actualHeight).toBeLessThan(heightLessThan - 1, 'heightLessThan' + msgPlus);
};
13 changes: 5 additions & 8 deletions test/jasmine/tests/legend_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var failTest = require('../assets/fail_test');
var delay = require('../assets/delay');
var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var assertPlotSize = require('../assets/custom_assertions').assertPlotSize;

describe('legend defaults', function() {
'use strict';
Expand Down Expand Up @@ -530,30 +531,26 @@ describe('legend relayout update', function() {
width: 500
}});

function plotWidth() {
return d3.select('.ygrid').node().getBoundingClientRect().width;
}

Plotly.newPlot(gd, mockCopy.data, mockCopy.layout)
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(1);
// check that the margins changed
expect(plotWidth()).toBeLessThan(400);
assertPlotSize({widthLessThan: 400});
return Plotly.relayout(gd, {showlegend: false});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(0);
expect(plotWidth()).toBe(400);
assertPlotSize({width: 400});
return Plotly.relayout(gd, {showlegend: true});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(1);
expect(plotWidth()).toBeLessThan(400);
assertPlotSize({widthLessThan: 400});
return Plotly.relayout(gd, {'legend.x': 0.7});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(1);
expect(plotWidth()).toBe(400);
assertPlotSize({width: 400});
})
.catch(failTest)
.then(done);
Expand Down
21 changes: 5 additions & 16 deletions test/jasmine/tests/range_selector_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ var failTest = require('../assets/fail_test');
var getRectCenter = require('../assets/get_rect_center');
var mouseEvent = require('../assets/mouse_event');
var setConvert = require('@src/plots/cartesian/set_convert');
var assertPlotSize = require('../assets/custom_assertions').assertPlotSize;


describe('range selector defaults:', function() {
Expand Down Expand Up @@ -620,37 +621,25 @@ describe('range selector automargin', function() {

afterEach(destroyGraphDiv);

function plotWidth() {
return d3.selectAll('.ygrid').node().getBoundingClientRect().width;
}

function plotHeight() {
return d3.selectAll('.xgrid').node().getBoundingClientRect().height;
}

it('updates automargin when hiding, showing, or moving', function(done) {
expect(plotWidth()).toBe(400);
expect(plotHeight()).toBe(300);
assertPlotSize({width: 400, height: 300}, 'initial');

Plotly.relayout(gd, {
'xaxis.rangeselector.y': 1.3,
'xaxis.rangeselector.xanchor': 'center'
})
.then(function() {
expect(plotWidth()).toBeLessThan(400);
expect(plotHeight()).toBeLessThan(300);
assertPlotSize({widthLessThan: 400, heightLessThan: 300}, 'moved');

return Plotly.relayout(gd, {'xaxis.rangeselector.visible': false});
})
.then(function() {
expect(plotWidth()).toBe(400);
expect(plotHeight()).toBe(300);
assertPlotSize({width: 400, height: 300}, 'hidden');

return Plotly.relayout(gd, {'xaxis.rangeselector.visible': true});
})
.then(function() {
expect(plotWidth()).toBeLessThan(400);
expect(plotHeight()).toBeLessThan(300);
assertPlotSize({widthLessThan: 400, heightLessThan: 300}, 'reshow');
})
.catch(failTest)
.then(done);
Expand Down
13 changes: 5 additions & 8 deletions test/jasmine/tests/range_slider_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ var destroyGraphDiv = require('../assets/destroy_graph_div');
var mouseEvent = require('../assets/mouse_event');
var supplyAllDefaults = require('../assets/supply_defaults');
var failTest = require('../assets/fail_test');
var assertPlotSize = require('../assets/custom_assertions').assertPlotSize;

var TOL = 6;

Expand Down Expand Up @@ -363,10 +364,6 @@ describe('Rangeslider visibility property', function() {

afterEach(destroyGraphDiv);

function plotHeight() {
return d3.select('.xgrid').node().getBoundingClientRect().height;
}

function defaultLayout(opts) {
return Lib.extendDeep({
width: 500,
Expand All @@ -380,7 +377,7 @@ describe('Rangeslider visibility property', function() {
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).not.toBeDefined();
expect(plotHeight()).toBe(400);
assertPlotSize({height: 400});
})
.catch(failTest)
.then(done);
Expand All @@ -394,7 +391,7 @@ describe('Rangeslider visibility property', function() {
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).toBeDefined();
expect(plotHeight()).toBeLessThan(400);
assertPlotSize({heightLessThan: 400});
})
.catch(failTest)
.then(done);
Expand All @@ -409,7 +406,7 @@ describe('Rangeslider visibility property', function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).toBeDefined();
expect(countRangeSliderClipPaths()).toEqual(1);
expect(plotHeight()).toBeLessThan(400);
assertPlotSize({heightLessThan: 400});
})
.catch(failTest)
.then(done);
Expand All @@ -431,7 +428,7 @@ describe('Rangeslider visibility property', function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).not.toBeDefined();
expect(countRangeSliderClipPaths()).toEqual(0);
expect(plotHeight()).toBe(400);
assertPlotSize({height: 400});
})
.catch(failTest)
.then(done);
Expand Down
5 changes: 5 additions & 0 deletions test/jasmine/tests/sliders_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');
var failTest = require('../assets/fail_test');
var delay = require('../assets/delay');
var assertPlotSize = require('../assets/custom_assertions').assertPlotSize;

describe('sliders defaults', function() {
'use strict';
Expand Down Expand Up @@ -324,12 +325,14 @@ describe('sliders interactions', function() {
it('should draw only visible sliders', function(done) {
expect(gd._fullLayout._pushmargin['slider-0']).toBeDefined();
expect(gd._fullLayout._pushmargin['slider-1']).toBeDefined();
assertPlotSize({heightLessThan: 270}, 'initial');

Plotly.relayout(gd, 'sliders[0].visible', false).then(function() {
assertNodeCount('.' + constants.groupClassName, 1);
expect(gd._fullLayout._pushmargin['slider-0']).toBeUndefined();
expect(gd._fullLayout._pushmargin['slider-1']).toBeDefined();
expect(gd.layout.sliders.length).toEqual(2);
assertPlotSize({heightLessThan: 270}, 'hide 0');

return Plotly.relayout(gd, 'sliders[1]', null);
})
Expand All @@ -338,6 +341,7 @@ describe('sliders interactions', function() {
expect(gd._fullLayout._pushmargin['slider-0']).toBeUndefined();
expect(gd._fullLayout._pushmargin['slider-1']).toBeUndefined();
expect(gd.layout.sliders.length).toEqual(1);
assertPlotSize({height: 270}, 'delete 1');

return Plotly.relayout(gd, {
'sliders[0].visible': true,
Expand All @@ -347,6 +351,7 @@ describe('sliders interactions', function() {
assertNodeCount('.' + constants.groupClassName, 1);
expect(gd._fullLayout._pushmargin['slider-0']).toBeDefined();
expect(gd._fullLayout._pushmargin['slider-1']).toBeUndefined();
assertPlotSize({heightLessThan: 270}, 'reshow 0');

return Plotly.relayout(gd, {
'sliders[1]': {
Expand Down