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

Layout grids #2399

Merged
merged 12 commits into from
Feb 26, 2018
63 changes: 55 additions & 8 deletions src/lib/coerce.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,19 +257,56 @@ exports.valObjectMeta = {
'An {array} of plot information.'
].join(' '),
requiredOpts: ['items'],
otherOpts: ['dflt', 'freeLength'],
// set dimensions=2 for a 2D array
// `items` may be a single object instead of an array, in which case
// `freeLength` must be true.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I needed 1D and 2D arbitrary-length info_array for specifying axes and/or subplots for the grid. I didn't actually end up using coerce with these, since there's validation involved where allowed values depend on both the available axes/subplots and on earlier elements in the same array; but it's still important for documentation, and for Plotly.react it's important that these are marked as info_array and not data_array.

Anyway, they work, in case we find a need in the future... tested below.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good stuff!

Would you mind updating the updatemenus and sliders attributes

image

so that they use items: {} instead of the not-as-general items: [{}, {}, {}].

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These ones are for function arguments, and we know that the functions being called have a maximum of 3 arguments. The [{}, {}, {}] form ensures we never go past three args. I suppose it probably wouldn't hurt to change it, but I didn't feel like taking that risk as I'm not sure how well tested the edge cases are for these components.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point here about args being at-most a three-item array. Let's keep that as it 👌

But this has me thinking: should we change attributes like ticktext and tickvals to free-length info_array instead of their current data_array val type? That way, we could get rid of this ugly hack.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should we change attributes like ticktext and tickvals to free-length info_array instead of their current data_array val type?

Hmm, possibly... would certainly be nice to keep data_array out of layout, but those are potentially long enough (and likely enough to be connected to a data source) to still warrant being called data_array so I'm a bit ambivalent about it. We could discuss this in a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could discuss this in a separate issue.

Sounds good 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's continue that discussion in -> #1894

otherOpts: ['dflt', 'freeLength', 'dimensions'],
coerceFunction: function(v, propOut, dflt, opts) {

// simplified coerce function just for array items
function coercePart(v, opts, dflt) {
var out;
var propPart = {set: function(v) { out = v; }};

if(dflt === undefined) dflt = opts.dflt;

exports.valObjectMeta[opts.valType].coerceFunction(v, propPart, dflt, opts);

return 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.

I had change this section to support non-array items - but also for existing info_array attributes this should be substantially faster than using regular coerce for each of the elements, since we're not constructing fake attribute strings and using them in the multiple nestedProperty constructs that coerce does.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amazing thanks!

}

var twoD = opts.dimensions === 2;

if(!Array.isArray(v)) {
propOut.set(dflt);
return;
}

var items = opts.items,
vOut = [];
var items = opts.items;
var vOut = [];
var arrayItems = Array.isArray(items);
var len = arrayItems ? items.length : v.length;

var i, j, len2, vNew;

dflt = Array.isArray(dflt) ? dflt : [];

for(var i = 0; i < items.length; i++) {
exports.coerce(v, vOut, items, '[' + i + ']', dflt[i]);
if(twoD) {
for(i = 0; i < len; i++) {
vOut[i] = [];
var row = Array.isArray(v[i]) ? v[i] : [];
len2 = arrayItems ? items[i].length : row.length;
for(j = 0; j < len2; j++) {
vNew = coercePart(row[j], arrayItems ? items[i][j] : items, (dflt[i] || [])[j]);
if(vNew !== undefined) vOut[i][j] = vNew;
}
}
}
else {
for(i = 0; i < len; i++) {
vNew = coercePart(v[i], arrayItems ? items[i] : items, dflt[i]);
if(vNew !== undefined) vOut[i] = vNew;
}
}

propOut.set(vOut);
Expand All @@ -278,15 +315,25 @@ exports.valObjectMeta = {
if(!Array.isArray(v)) return false;

var items = opts.items;
var arrayItems = Array.isArray(items);
var twoD = opts.dimensions === 2;

// when free length is off, input and declared lengths must match
if(!opts.freeLength && v.length !== items.length) return false;

// valid when all input items are valid
for(var i = 0; i < v.length; i++) {
var isItemValid = exports.validate(v[i], opts.items[i]);

if(!isItemValid) return false;
if(twoD) {
if(!Array.isArray(v[i]) || (!opts.freeLength && v[i].length !== items[i].length)) {
return false;
}
for(var j = 0; j < v[i].length; j++) {
if(!exports.validate(v[i][j], arrayItems ? items[i][j] : items)) {
return false;
}
}
}
else if(!exports.validate(v[i], arrayItems ? items[i] : items)) return false;
}

return true;
Expand Down
19 changes: 11 additions & 8 deletions src/lib/regex.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,16 +8,19 @@

'use strict';

// Simple helper functions
// none of these need any external deps

/*
* make a regex for matching counter ids/names ie xaxis, xaxis2, xaxis10...
* eg: regexCounter('x')
* tail is an optional piece after the id
* eg regexCounter('scene', '.annotations') for scene2.annotations etc.
*
* @param {string} head: the head of the pattern, eg 'x' matches 'x', 'x2', 'x10' etc.
* 'xy' is a special case for cartesian subplots: it matches 'x2y3' etc
* @param {Optional(string)} tail: a fixed piece after the id
* eg counterRegex('scene', '.annotations') for scene2.annotations etc.
* @param {boolean} openEnded: if true, the string may continue past the match.
*/
exports.counter = function(head, tail, openEnded) {
return new RegExp('^' + head + '([2-9]|[1-9][0-9]+)?' +
(tail || '') + (openEnded ? '' : '$'));
var fullTail = (tail || '') + (openEnded ? '' : '$');
if(head === 'xy') {
return new RegExp('^x([2-9]|[1-9][0-9]+)?y([2-9]|[1-9][0-9]+)?' + fullTail);
}
return new RegExp('^' + head + '([2-9]|[1-9][0-9]+)?' + fullTail);
};
2 changes: 1 addition & 1 deletion src/lib/stats.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ var isNumeric = require('fast-isnumeric');
exports.aggNums = function(f, v, a, len) {
var i,
b;
if(!len) len = a.length;
if(!len || len > a.length) len = a.length;
if(!isNumeric(v)) v = false;
if(Array.isArray(a[0])) {
b = new Array(len);
Expand Down
22 changes: 19 additions & 3 deletions src/plot_api/plot_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,8 @@ function recurseIntoValObject(valObject, parts, i) {
}

// now recurse as far as we can. Occasionally we have an attribute
// setting an internal part below what's
// setting an internal part below what's in the schema; just return
// the innermost schema item we find.
for(; i < parts.length; i++) {
var newValObject = valObject[parts[i]];
if(Lib.isPlainObject(newValObject)) valObject = newValObject;
Expand All @@ -398,8 +399,23 @@ function recurseIntoValObject(valObject, parts, i) {
else if(valObject.valType === 'info_array') {
i++;
var index = parts[i];
if(!isIndex(index) || index >= valObject.items.length) return false;
valObject = valObject.items[index];
if(!isIndex(index)) return false;

var items = valObject.items;
if(Array.isArray(items)) {
if(index >= items.length) return false;
if(valObject.dimensions === 2) {
i++;
if(parts.length === i) return valObject;
var index2 = parts[i];
if(!isIndex(index2)) return false;
valObject = items[index][index2];
}
else valObject = items[index];
}
else {
valObject = items;
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/plots/cartesian/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

'use strict';
var counterRegex = require('../../lib').counterRegex;
var counterRegex = require('../../lib/regex').counter;


module.exports = {
Expand Down
3 changes: 2 additions & 1 deletion src/plots/cartesian/layout_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
var positioningOptions = {
letter: axLetter,
counterAxes: counterAxes[axLetter],
overlayableAxes: overlayableAxes
overlayableAxes: overlayableAxes,
grid: layoutOut.grid
};

handlePositionDefaults(axLayoutIn, axLayoutOut, coerce, positioningOptions);
Expand Down
37 changes: 27 additions & 10 deletions src/plots/cartesian/position_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,26 +15,43 @@ var Lib = require('../../lib');


module.exports = function handlePositionDefaults(containerIn, containerOut, coerce, options) {
var counterAxes = options.counterAxes || [],
overlayableAxes = options.overlayableAxes || [],
letter = options.letter;
var counterAxes = options.counterAxes || [];
var overlayableAxes = options.overlayableAxes || [];
var letter = options.letter;
var grid = options.grid;

var dfltAnchor, dfltDomain, dfltSide, dfltPosition;

if(grid) {
dfltDomain = grid._domains[letter][grid._axisMap[containerOut._id]];
dfltAnchor = grid._anchors[containerOut._id];
if(dfltDomain) {
dfltSide = grid[letter + 'side'].split(' ')[0];
dfltPosition = grid.domain[letter][dfltSide === 'right' || dfltSide === 'top' ? 1 : 0];
}
}

// Even if there's a grid, this axis may not be in it - fall back on non-grid defaults
dfltDomain = dfltDomain || [0, 1];
dfltAnchor = dfltAnchor || (isNumeric(containerIn.position) ? 'free' : (counterAxes[0] || 'free'));
dfltSide = dfltSide || (letter === 'x' ? 'bottom' : 'left');
dfltPosition = dfltPosition || 0;

var anchor = Lib.coerce(containerIn, containerOut, {
anchor: {
valType: 'enumerated',
values: ['free'].concat(counterAxes),
dflt: isNumeric(containerIn.position) ? 'free' :
(counterAxes[0] || 'free')
dflt: dfltAnchor
}
}, 'anchor');

if(anchor === 'free') coerce('position');
if(anchor === 'free') coerce('position', dfltPosition);

Lib.coerce(containerIn, containerOut, {
side: {
valType: 'enumerated',
values: letter === 'x' ? ['bottom', 'top'] : ['left', 'right'],
dflt: letter === 'x' ? 'bottom' : 'left'
dflt: dfltSide
}
}, 'side');

Expand All @@ -54,9 +71,9 @@ module.exports = function handlePositionDefaults(containerIn, containerOut, coer
// in ax.setscale()... but this means we still need (imperfect) logic
// in the axes popover to hide domain for the overlaying axis.
// perhaps I should make a private version _domain that all axes get???
var domain = coerce('domain');
if(domain[0] > domain[1] - 0.01) containerOut.domain = [0, 1];
Lib.noneOrAll(containerIn.domain, containerOut.domain, [0, 1]);
var domain = coerce('domain', dfltDomain);
if(domain[0] > domain[1] - 0.01) containerOut.domain = dfltDomain;
Lib.noneOrAll(containerIn.domain, containerOut.domain, dfltDomain);
}

coerce('layer');
Expand Down
62 changes: 60 additions & 2 deletions src/plots/domain_attributes.js → src/plots/domain.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ var extendFlat = require('../lib/extend').extendFlat;
* opts.trace: set to true for trace containers
* @param {string}
* opts.editType: editType for all pieces
* @param {boolean}
* opts.noGridCell: set to true to omit `row` and `column`
*
* @param {object} extra
* @param {string}
Expand All @@ -29,7 +31,7 @@ var extendFlat = require('../lib/extend').extendFlat;
*
* @return {object} attributes object containing {x,y} as specified
*/
module.exports = function(opts, extra) {
exports.attributes = function(opts, extra) {
opts = opts || {};
extra = extra || {};

Expand All @@ -48,7 +50,7 @@ module.exports = function(opts, extra) {
var contPart = opts.trace ? 'trace ' : 'subplot ';
var descPart = extra.description ? ' ' + extra.description : '';

return {
var out = {
x: extendFlat({}, base, {
description: [
'Sets the horizontal domain of this ',
Expand All @@ -69,4 +71,60 @@ module.exports = function(opts, extra) {
}),
editType: opts.editType
};

if(!opts.noGridCell) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see any noGridCell options being set. 🔪 ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

noGridCell shows up in the domain for the grid itself - you can't put the grid in itself! 😄

out.row = {
valType: 'integer',
min: 0,
Copy link
Contributor

Choose a reason for hiding this comment

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

Two suggestions:

  • It might be worth adding a dflt: 0, so that domain: {column: 2} defaults to subplot xy2?
  • It might be nice to allow domain: {row: ''} to match the 'x'/'x2'/'x3' counter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be worth adding a dflt: 0, so that domain: {column: 2} defaults to subplot xy2?

Yeah, makes sense - if you have a grid, by default all subplots should go in it. You can always set explicit domain.x and domain.y anyhow, this is only setting the defaults for those attributes -> 4b43e35

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It might be nice to allow domain: {row: ''} to match the 'x'/'x2'/'x3' counter

As discussed offline, I omitted this one (though if you say '' you'll get 0 now anyway with the change ^^ so the result is the same)

role: 'info',
editType: opts.editType,
description: [
'If there is a layout grid, use the domain ',
'for this row in the grid for this ',
namePart,
contPart,
'.',
descPart
].join('')
};
out.column = {
valType: 'integer',
min: 0,
role: 'info',
editType: opts.editType,
description: [
'If there is a layout grid, use the domain ',
'for this column in the grid for this ',
namePart,
contPart,
'.',
descPart
].join('')
};
}

return out;
};

exports.defaults = function(containerOut, layout, coerce, dfltDomains) {
var dfltX = (dfltDomains && dfltDomains.x) || [0, 1];
var dfltY = (dfltDomains && dfltDomains.y) || [0, 1];

var grid = layout.grid;
if(grid) {
var column = coerce('domain.column');
if(column !== undefined) {
if(column < grid.columns) dfltX = grid._domains.x[column];
else delete containerOut.domain.column;
}

var row = coerce('domain.row');
if(row !== undefined) {
if(row < grid.rows) dfltY = grid._domains.y[row];
else delete containerOut.domain.row;
}
}

coerce('domain.x', dfltX);
coerce('domain.y', dfltY);
};
2 changes: 1 addition & 1 deletion src/plots/geo/layout/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
'use strict';

var colorAttrs = require('../../../components/color/attributes');
var domainAttrs = require('../../domain_attributes');
var domainAttrs = require('../../domain').attributes;
var constants = require('../constants');
var overrideAll = require('../../../plot_api/edit_types').overrideAll;

Expand Down
2 changes: 1 addition & 1 deletion src/plots/gl3d/layout/layout_attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
'use strict';

var gl3dAxisAttrs = require('./axis_attributes');
var domainAttrs = require('../../domain_attributes');
var domainAttrs = require('../../domain').attributes;
var extendFlat = require('../../../lib/extend').extendFlat;
var counterRegex = require('../../../lib').counterRegex;

Expand Down
Loading