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
20 changes: 19 additions & 1 deletion test/jasmine/tests/legend_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -524,18 +524,36 @@ describe('legend relayout update', function() {
afterEach(destroyGraphDiv);

it('should hide and show the legend', function(done) {
var mockCopy = Lib.extendDeep({}, mock);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this commit locking down fixes from this PR, or just improving our test case coverage?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Pretty sure at least one of these was broken before this PR, but I don't recall which anymore.

var mockCopy = Lib.extendDeep({}, mock, {layout: {
legend: {x: 1.1, xanchor: 'left'},
margin: {l: 50, r: 50},
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);
return Plotly.relayout(gd, {showlegend: false});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(0);
expect(plotWidth()).toBe(400);
return Plotly.relayout(gd, {showlegend: true});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(1);
expect(plotWidth()).toBeLessThan(400);
return Plotly.relayout(gd, {'legend.x': 0.7});
})
.then(function() {
expect(d3.selectAll('g.legend').size()).toBe(1);
expect(plotWidth()).toBe(400);
})
.catch(failTest)
.then(done);
Expand Down
60 changes: 60 additions & 0 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 @@ -597,3 +597,63 @@ describe('range selector interactions:', function() {
.then(done);
});
});

describe('range selector automargin', function() {
'use strict';

var mock = require('@mocks/range_selector.json');

var gd, mockCopy;

beforeEach(function(done) {
gd = createGraphDiv();
mockCopy = Lib.extendDeep({}, mock, {layout: {
width: 500,
height: 500,
margin: {l: 50, r: 50, t: 100, b: 100}
}});

Plotly.plot(gd, mockCopy.data, mockCopy.layout)
.catch(failTest)
.then(done);
});

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);

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

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

return Plotly.relayout(gd, {'xaxis.rangeselector.visible': true});
})
.then(function() {
expect(plotWidth()).toBeLessThan(400);
expect(plotHeight()).toBeLessThan(300);
})
.catch(failTest)
.then(done);
});

});
30 changes: 23 additions & 7 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 @@ -363,38 +363,53 @@ describe('Rangeslider visibility property', function() {

afterEach(destroyGraphDiv);

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

function defaultLayout(opts) {
return Lib.extendDeep({
width: 500,
height: 600,
margin: {l: 50, r: 50, t: 100, b: 100}
}, opts || {});
}

it('should not add the slider to the DOM by default', function(done) {
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], defaultLayout())
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).not.toBeDefined();
expect(plotHeight()).toBe(400);
})
.catch(failTest)
.then(done);
});

it('should add the slider if rangeslider is set to anything', function(done) {
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], defaultLayout())
.then(function() {
return Plotly.relayout(gd, 'xaxis.rangeslider', 'exists');
})
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).toBeDefined();
expect(plotHeight()).toBeLessThan(400);
})
.catch(failTest)
.then(done);
});

it('should add the slider if visible changed to `true`', function(done) {
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], {})
Plotly.plot(gd, [{ x: [1, 2, 3], y: [2, 3, 4] }], defaultLayout())
.then(function() {
return Plotly.relayout(gd, 'xaxis.rangeslider.visible', true);
})
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).toBeDefined();
expect(countRangeSliderClipPaths()).toEqual(1);
expect(plotHeight()).toBeLessThan(400);
})
.catch(failTest)
.then(done);
Expand All @@ -404,18 +419,19 @@ describe('Rangeslider visibility property', function() {
Plotly.plot(gd, [{
x: [1, 2, 3],
y: [2, 3, 4]
}], {
}], defaultLayout({
xaxis: {
rangeslider: { visible: true }
}
})
}))
.then(function() {
return Plotly.relayout(gd, 'xaxis.rangeslider.visible', false);
})
.then(function() {
var rangeSlider = getRangeSlider();
expect(rangeSlider).not.toBeDefined();
expect(countRangeSliderClipPaths()).toEqual(0);
expect(plotHeight()).toBe(400);
})
.catch(failTest)
.then(done);
Expand All @@ -434,11 +450,11 @@ describe('Rangeslider visibility property', function() {
type: 'bar',
x: [1, 2, 3],
y: [2, 5, 2]
}], {
}], defaultLayout({
xaxis: {
rangeslider: { visible: true }
}
})
}))
.then(function() {
expect(count('g.scatterlayer > g.trace')).toEqual(1);
expect(count('g.barlayer > g.trace')).toEqual(1);
Expand Down