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.

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

image: improve layout defaults #4307

Merged
merged 14 commits into from
Oct 29, 2019
Merged

image: improve layout defaults #4307

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
5c42db8
image: layout defaults: add scaleanchor, fix logic for reverse axis
antoinerg Oct 25, 2019
8184a8d
add and image mock to our mock_lists
antoinerg Oct 25, 2019
d71c3b3
image: :hocho: scaleanchor: null and improve style
antoinerg Oct 28, 2019
97a8d24
image: test smart layout defaults via supplyAllDefaults instead of plot
antoinerg Oct 28, 2019
2053c58
image: fix code style by breaking long line into multiple ones
antoinerg Oct 28, 2019
2b0d3c3
image: update description to document the layout defaults
antoinerg Oct 28, 2019
2306880
image: smart default layout to constrain y axis to domain
antoinerg Oct 28, 2019
6ad80ab
image: document smart layout default to constrain to domain
antoinerg Oct 28, 2019
8ef17c3
image: apply layout defaults if an image is present at all
antoinerg Oct 29, 2019
d1eca2c
image: test that axis default constrain: 'domain' is applied to x axis
antoinerg Oct 29, 2019
31dbc8e
fix to make sure axis layout defaults are used when no image present
antoinerg Oct 29, 2019
29cf98f
fix to set scaleanchorDflt and constrainDflt to undefined when no ima…
antoinerg Oct 29, 2019
4233619
Merge pull request #4313 from plotly/image-constrain
antoinerg Oct 29, 2019
408b46b
image: improve accuracy of test's description by :hocho: the word "only"
antoinerg Oct 29, 2019
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
8 changes: 5 additions & 3 deletions src/plots/cartesian/constraints.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@ var concatExtremes = require('./autorange').concatExtremes;
var ALMOST_EQUAL = require('../../constants/numerical').ALMOST_EQUAL;
var FROM_BL = require('../../constants/alignment').FROM_BL;

exports.handleConstraintDefaults = function(containerIn, containerOut, coerce, allAxisIds, layoutOut) {
exports.handleConstraintDefaults = function(containerIn, containerOut, coerce, opts) {
var allAxisIds = opts.allAxisIds;
var layoutOut = opts.layoutOut;
var constraintGroups = layoutOut._axisConstraintGroups;
var matchGroups = layoutOut._axisMatchGroups;
var axId = containerOut._id;
Expand Down Expand Up @@ -53,14 +55,14 @@ exports.handleConstraintDefaults = function(containerIn, containerOut, coerce, a
// 'matches' wins over 'scaleanchor' (for now)
var scaleanchor, scaleOpts;

if(!matches && containerIn.scaleanchor && !(containerOut.fixedrange && constrain !== 'domain')) {
if(!matches && (containerIn.scaleanchor || opts.scaleanchorDflt) && !(containerOut.fixedrange && constrain !== 'domain')) {
etpinard marked this conversation as resolved.
Show resolved Hide resolved
scaleOpts = getConstraintOpts(constraintGroups, thisID, allAxisIds, layoutOut, constrain);
scaleanchor = Lib.coerce(containerIn, containerOut, {
scaleanchor: {
valType: 'enumerated',
values: scaleOpts.linkableAxes || []
}
}, 'scaleanchor');
}, 'scaleanchor', opts.scaleanchorDflt);
etpinard marked this conversation as resolved.
Show resolved Hide resolved
}

if(matches) {
Expand Down
32 changes: 21 additions & 11 deletions src/plots/cartesian/layout_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,10 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
var yaMayHide = {};
var xaMustDisplay = {};
var yaMustDisplay = {};
var yaMustForward = {};
var yaMayBackward = {};
var yaMustNotReverse = {};
archmoj marked this conversation as resolved.
Show resolved Hide resolved
var yaMayReverse = {};
var yaMustNotScaleanchor = {};
var yaMayScaleanchor = {};
var outerTicks = {};
var noGrids = {};
var i, j;
Expand Down Expand Up @@ -74,24 +76,24 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
if(trace.type === 'funnel') {
if(trace.orientation === 'h') {
if(xaName) xaMayHide[xaName] = true;
if(yaName) yaMayBackward[yaName] = true;
if(yaName) yaMayReverse[yaName] = true;
} else {
if(yaName) yaMayHide[yaName] = true;
}
yaMustNotScaleanchor[yaName] = true;
} else if(trace.type === 'image') {
if(yaName) yaMayReverse[yaName] = true;
if(yaName) yaMayScaleanchor[yaName] = true;
etpinard marked this conversation as resolved.
Show resolved Hide resolved
} else {
if(yaName) {
yaMustDisplay[yaName] = true;
yaMustForward[yaName] = true;
yaMustNotReverse[yaName] = true;
yaMustNotScaleanchor[yaName] = true;
}

if(!traceIs(trace, 'carpet') || (trace.type === 'carpet' && !trace._cheater)) {
if(xaName) xaMustDisplay[xaName] = true;
}

if(trace.type === 'image') {
if(yaName) yaMustForward[yaName] = false;
if(yaName) yaMayBackward[yaName] = true;
}
}

// Two things trigger axis visibility:
Expand Down Expand Up @@ -197,7 +199,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
(axLetter === 'y' && !yaMustDisplay[axName] && yaMayHide[axName]);

var reverseDflt =
(axLetter === 'y' && !yaMustForward[axName] && yaMayBackward[axName]);
(axLetter === 'y' && !yaMustNotReverse[axName] && yaMayReverse[axName]);

var defaultOptions = {
letter: axLetter,
Expand Down Expand Up @@ -299,7 +301,15 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
axLayoutIn = layoutIn[axName];
axLayoutOut = layoutOut[axName];

handleConstraintDefaults(axLayoutIn, axLayoutOut, coerce, allAxisIds, layoutOut);
var scaleanchorDflt = null;
if(axLetter === 'y' && !axLayoutIn.hasOwnProperty('scaleanchor') && !yaMustNotScaleanchor[axName] && yaMayScaleanchor[axName]) {
etpinard marked this conversation as resolved.
Show resolved Hide resolved
scaleanchorDflt = axLayoutOut.anchor;
}
handleConstraintDefaults(axLayoutIn, axLayoutOut, coerce, {
allAxisIds: allAxisIds,
layoutOut: layoutOut,
scaleanchorDflt: scaleanchorDflt
});
}

for(i = 0; i < matchGroups.length; i++) {
Expand Down
Binary file modified test/image/baselines/image_axis_reverse.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/image_axis_type.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion test/image/mocks/image_adventurer.json
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion test/image/mocks/image_axis_type.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
"width": 400, "height": 600, "title": {"text": "Image on categorical and log axes"},
"grid": {"rows": 2, "columns": 1, "pattern": "independent"},
"xaxis2": {"type": "log"},
"yaxis2": {"type": "log"}
"yaxis": {"scaleanchor": null},
"yaxis2": {"type": "log", "scaleanchor": null}
Copy link
Contributor

Choose a reason for hiding this comment

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

null isn't a valid scaleanchor value. Please remove all the scalanchor: 'null' lines from the mocks. If you want to test that scaleanchor: null does the right thing, use a jasmine test.

Copy link
Contributor

Choose a reason for hiding this comment

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

... this mean you'll have to update a few baselines, I hope you don't mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in d71c3b3

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks to the new baselines I realised that the constrain default is 'range'.

If I recall correctly, both @emmanuelle and @nicolaskruchten prefered the look of constrain: 'domain' for image traces.

Personally, I think constrain: 'range' looks just fine and it's probably not worth adding more smart (aka magic) defaults in this case.

Would it be ok to keep constrain: 'range' as default here?

Copy link
Contributor Author

@antoinerg antoinerg Oct 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Mmm 🤔 I'm not sure. I think constrain: 'domain' looks a bit better when a single image trace is present. I don't know how I feel about adding some more magic defaults. I'll let @emmanuelle and @nicolaskruchten chime in.

@etpinard About the smart defaults in image, where should we document them? I guess within the trace description in its index.js file?

Copy link
Contributor

Choose a reason for hiding this comment

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

About the smart defaults in image, where should we document them? I guess within the trace description in its index.js file?

Yep, in the image/index.js description would be 👌

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 2b0d3c3

Let's wait on @emmanuelle and @nicolaskruchten about constrain: 'domain'

}
}
2 changes: 1 addition & 1 deletion test/image/mocks/image_cat.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"layout": {
"width": 400, "height": 400, "yaxis": {"scaleanchor": "x"}, "margin": {"t":25, "b": 25, "r": 25, "l": 40}
"width": 400, "height": 400, "yaxis": {"scaleanchor": "x", "autorange": "reversed"}, "margin": {"t":25, "b": 25, "r": 25, "l": 40}
},
"data": [
{"x":[50, 150, 350, 50], "y":[350, 23, 100, 350]},
Expand Down
2 changes: 2 additions & 0 deletions test/image/mocks/image_colormodel.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
}],
"layout": {
"grid": {"rows": 2, "columns": 1, "pattern": "independent"},
"yaxis": {"scaleanchor": null},
"yaxis2": {"scaleanchor": null},
"width": 600,
"height": 400
}
Expand Down
1 change: 1 addition & 0 deletions test/image/mocks/image_opacity.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
"z": [[[255, 0, 0], [0, 255, 0], [0, 0, 255]]]
}],
"layout": {
"yaxis": {"scaleanchor": null},
"width": 400, "height": 400,
"title": {
"text": "image with opacity 0.1"
Expand Down
1 change: 1 addition & 0 deletions test/image/mocks/image_with_gaps.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"zmax": [1, 1, 1]
}],
"layout": {
"yaxis": {"scaleanchor": null},
"width": 400, "height": 400, "title": {"text": "Image with missing pixels"}
}
}
8 changes: 8 additions & 0 deletions test/image/mocks/image_zmin_zmax.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,14 @@
"layout": {
"width": 400,
"height": 800,
"yaxis": {"scaleanchor": null},
"yaxis2": {"scaleanchor": null},
"yaxis3": {"scaleanchor": null},
"yaxis4": {"scaleanchor": null},
"yaxis5": {"scaleanchor": null},
"yaxis6": {"scaleanchor": null},
"yaxis7": {"scaleanchor": null},
"yaxis8": {"scaleanchor": null},
etpinard marked this conversation as resolved.
Show resolved Hide resolved
"grid": {
"rows": 4,
"columns": 2,
Expand Down
1 change: 1 addition & 0 deletions test/jasmine/assets/mock_lists.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ var svgMockList = [
['finance_style', require('@mocks/finance_style.json')],
['geo_first', require('@mocks/geo_first.json')],
['indicator_bignumber', require('@mocks/indicator_bignumber.json')],
['image_adventurer', require('@mocks/image_adventurer.json')],
['layout_image', require('@mocks/layout_image.json')],
['layout-colorway', require('@mocks/layout-colorway.json')],
['multicategory', require('@mocks/multicategory.json')],
Expand Down
58 changes: 57 additions & 1 deletion test/jasmine/tests/image_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,62 @@ describe('image supplyDefaults', function() {
});
});

describe('image smart layout defaults', function() {
var gd;
beforeEach(function() {
gd = createGraphDiv();
});

afterEach(destroyGraphDiv);

it('should reverse yaxis if only images are present', function(done) {
Plotly.newPlot(gd, [{type: 'image', z: [[[255, 0, 0]]]}])
etpinard marked this conversation as resolved.
Show resolved Hide resolved
.then(function(gd) {
expect(gd._fullLayout.yaxis.range[0]).toBeGreaterThan(gd._fullLayout.yaxis.range[1]);
})
.catch(failTest)
.then(done);
});

it('should NOT reverse yaxis if another trace is present', function(done) {
Plotly.newPlot(gd, [{type: 'image', z: [[[255, 0, 0]]]}, {type: 'scatter', y: [5, 3, 2]}])
.then(function(gd) {
expect(gd._fullLayout.yaxis.range[1]).toBeGreaterThan(gd._fullLayout.yaxis.range[0]);
})
.catch(failTest)
.then(done);
});

it('should set scaleanchor to make square pixels if only images are present', function(done) {
Plotly.newPlot(gd, [{type: 'image', z: [[[255, 0, 0]]]}])
.then(function(gd) {
expect(gd._fullLayout.yaxis.scaleanchor).toBe('x');
})
.catch(failTest)
.then(done);
});

it('should NOT set scaleanchor if another trace is present', function(done) {
Plotly.newPlot(gd, [{type: 'image', z: [[[255, 0, 0]]]}, {type: 'scatter', y: [5, 3, 2]}])
.then(function(gd) {
expect(gd._fullLayout.yaxis.scaleanchor).toBe(undefined);
})
.catch(failTest)
.then(done);
});

it('should NOT set scaleanchor if it\'s already defined', function(done) {
Plotly.newPlot(gd, [
{type: 'image', z: [[[255, 0, 0]]]}, {type: 'scatter', y: [5, 3, 2]}
], {yaxis: {scaleanchor: 'x3'}})
.then(function(gd) {
expect(gd._fullLayout.yaxis.scaleanchor).toBe(undefined);
})
.catch(failTest)
.then(done);
});
});

describe('image plot', function() {
'use strict';

Expand Down Expand Up @@ -444,7 +500,7 @@ describe('image hover:', function() {
zmax: [1, 1, 1],
text: [['A', 'B', 'C'], ['D', 'E', 'F']],
hovertemplate: '%{text}<extra></extra>'
}], layout: {width: 400, height: 400}};
}], layout: {width: 400, height: 400, yaxis: {scaleanchor: null}}};
etpinard marked this conversation as resolved.
Show resolved Hide resolved

Plotly.newPlot(gd, mockCopy)
.then(function() {_hover(140, 200);})
Expand Down