Templates #2761

alexcjohnson · 2018-06-27T21:29:16Z

The first 6 commits - up through 3dee25c - are essentially prep work, mostly standardizing and simplifying our handling of container arrays.

Then there's one large commit 5cca8d3 that implements the coerce-level templating that I settled on, after first trying to merge templates into data and layout directly at the beginning of supplyDefaults, which failed for a variety of reasons.

After that are a bunch of cleanup commits, like 8e2a321 making array objects (which may not exist in layout if they came from named template items) work with GUI editing.

TODO:

Jasmine tests - the committed ones right now are still just the skeleton from @bpostlethwaite's initial work and don't actually apply.
Checking if the template is compatible with the plot it was applied to. This is a bit ill-defined, but I guess means mostly do all the trace types and layout containers fit what's in the template.

@etpinard anything here look problematic to you?

rangeselector.buttons updatemenus.buttons slider.steps axes.tickformatstops mapbox.layers

rangeselector.buttons updatemenus.buttons parcoords & splom.dimensions (and DRY these a bit)

there may be other similar logic we need to update for templates, but these are the most readily apparent ones

etpinard · 2018-06-27T21:33:00Z

cc @nicolaskruchten this is happening 🎉

etpinard

I like the way this looks so far.

The coerce-level template logic appears pretty robust 💪

etpinard · 2018-06-27T22:13:06Z

src/plots/cartesian/layout_attributes.js

@@ -513,6 +513,16 @@ module.exports = {
    tickformatstops: {
        _isLinkedToArray: 'tickformatstop',

+        visible: {


Maybe enabled (similar to transforms) here would make more sense.

.... but maybe it isn't worth breaking symmetric with all other templated arrays 🤔

Right, enabled certainly makes more sense here. I thought about that but deferred to symmetry at the time. But the way this ended up working out, it would be easy to make visibilityAttr: 'enabled' or something an option to handleArrayContainerDefaults. I'll give that a try.

switched to enabled in fb489aa

I'm glad this wasn't too hard to implement 🎉

etpinard · 2018-06-27T22:14:23Z

test/jasmine/tests/mapbox_test.js

+        })
+        .then(function() {
+            expect(getLayerLength(gd)).toEqual(2);
+            expect(countVisibleLayers(gd)).toEqual(2);


Nice tests. Thanks!

etpinard · 2018-06-27T22:15:05Z

src/components/rangeselector/button_attributes.js

@@ -10,6 +10,12 @@


 module.exports = {
+    visible: {


Brilliant commit ✨

etpinard · 2018-06-27T22:17:11Z

src/traces/parcoords/merge_length.js

+ * @param {string} dataAttr: the attribute of each dimension containing the data
+ * @param {integer} len: an already-existing length from other attributes
+ */
+module.exports = function(traceOut, dimensions, dataAttr, len) {


Nicely done 🥇

etpinard · 2018-06-27T22:17:54Z

src/components/annotations/defaults.js

 var handleArrayContainerDefaults = require('../../plots/array_container_defaults');
-var handleAnnotationDefaults = require('./annotation_defaults');


YES. Thanks for doing this 💯

etpinard · 2018-06-27T22:24:39Z

src/plots/polar/layout_defaults.js

+        // Note: does not need template propagation, since coerceAxis is still
+        // based on the subplot-wide coerce function. Though it may be more
+        // efficient to make a new coerce function, then we *would* need to
+        // propagate the template.


thanks for the note 📚

etpinard · 2018-06-27T22:27:35Z

src/plots/array_container_defaults.js

@@ -34,7 +34,6 @@ var Template = require('../plot_api/plot_template');
 *          - parentObj {object} (as in closure)
 *          - opts {object} (as in closure)
 *          - itemOpts {object}
- *              - itemIsNotPlainObject {boolean}


Nice cleanup!

etpinard · 2018-06-27T22:28:38Z

src/plot_api/plot_template.js

@@ -12,7 +12,56 @@
 var Lib = require('../lib');
 var plotAttributes = require('../plots/attributes');

-var TEMPLATEITEMNAME = exports.TEMPLATEITEMNAME = 'templateitemname';
+var TEMPLATEITEMNAME = 'templateitemname';


I'm usually not a fan of three-word attribute names, but I can't really think of a better alternative, so 👍

…re 🎉

alexcjohnson · 2018-07-03T13:12:12Z

src/plots/cartesian/layout_attributes.js

+        // value back to the input - so it doesn't make sense to template this.
+        // TODO: should we prohibit this in `coerce` as well, or honor it if
+        // someone enters it explicitly?
+        _noTemplating: true,


I prevented makeTemplate from pulling out axis.type with this flag, as it seems more likely to cause problems than fix them, but I did NOT prevent coerce from using it if someone makes a template of manually and includes it (to say "always make this axis categorical" for example). Seem reasonable?

Seem reasonable?

Very reasonable 👌

alexcjohnson · 2018-07-03T13:14:14Z

src/plot_api/validate.js

@@ -397,6 +397,8 @@ function isInSchema(schema, key) {
 }

 function getNestedSchema(schema, key) {
+    if(key in schema) return schema[key];
+


Not part of templating, just noticed it as there's similar logic in templates and I was looking to see how you handled it here 🙈

alexcjohnson · 2018-07-03T13:16:54Z

test/image/mocks/shapes.json

@@ -18,7 +18,7 @@
        "yaxis2":{"title":"category","range":[0,1],"domain":[0.6,1],"anchor":"x2","type":"category","showgrid":false,"zeroline":false,"showticklabels":false},
        "height":400,
        "width":800,
-        "margin": {"l":20,"r":20,"top":10,"bottom":10,"pad":0},


I didn't want to change the image so just deleted these bad attrs uncovered when testing this fix.

Perhaps we should make a test that validates all our mocks, to ensure we're not accidentally encouraging mistaken usage like this? Not in this PR of course...

Perhaps we should make a test that validates all our mocks, to ensure we're not accidentally encouraging mistaken usage like this?

That's a great idea 👍

alexcjohnson · 2018-07-03T13:19:41Z

src/plot_api/plot_template.js

@@ -203,6 +203,8 @@ exports.arrayTemplater = function(container, name, inclusionAttr) {
        // it's explicitly marked visible - in which case it gets NO template,
        // not even the default.
        out[inclusionAttr] = itemIn[inclusionAttr] || false;
+        // special falsy value we can look for in validateTemplate
+        out._template = false;


This case is just special because we were told to look for a template by name and it wasn't found.

Otherwise _template can be null (no template was found but that's fine) or undefined (we don't need to propagate the template into this container).

etpinard · 2018-07-03T14:30:23Z

src/plot_api/validate.js

@@ -38,7 +38,7 @@ var isArrayOrTypedArray = Lib.isArrayOrTypedArray;
 *  - {string} msg
 *      error message (shown in console in logger config argument is enable)
 */
-module.exports = function valiate(data, layout) {
+module.exports = function validate(data, layout) {


good 👁️

etpinard · 2018-07-03T14:36:05Z

src/plot_api/template_api.js

+    else {
+        // TODO: any need to look deeper than the first level of layout?
+        // I don't think so, that gets all the subplot types which should be
+        // sufficient.


What about xaxis.rangeslider.yaxis2 from #2364?

Hmm, good point. You're right, though it's a lot of complication (for example for xaxis.rangeslider.yaxis we'd have to look for xaxis<N>.rangeslider.yaxis<M> for any combination of <N> and <M> and only generate an error if we don't find any of them. And the only time this would alert you that the template isn't appropriate is if the template has a xaxis<N>.rangeslider.yaxis<M> but does NOT have yaxis<M> - ie your template had opinions about how that y axis should work inside the rangeslider, but left the y axis itself untouched. This is of course possible, just seems very unlikely.

This is of course possible, just seems very unlikely.

Right. I don't this needs special handling. A comment about this would suffice.

actually wasn't all that hard - recursed into layout in 15931cf

etpinard · 2018-07-03T14:38:07Z

src/plot_api/template_api.js

+ *     to test. If omitted, we will look for a template already attached as the
+ *     plot's `layout.template` attribute.
+ *
+ * @returns {array} array of error objects each containing:


THanks for making this have a similar API as Plotly.validate !

One difference is I didn't make the object fields consistent - they all have code but each error, in fact even more specific than each code, includes whatever extra data it deems relevant. Given the nature of the issues (some apply to a specific trace, some to a path, some to a trace type but not a specific trace, etc... as opposed to Plotly.validate where you can basically always trace the issue back to a specific path) it seemed a bit to constraining to give them all the same fields.

etpinard · 2018-07-03T15:22:03Z

src/plots/cartesian/layout_attributes.js

+        // value back to the input - so it doesn't make sense to template this.
+        // Note: we do NOT prohibit this in `coerce`, so if someone enters a
+        // type in the template explicitly it will be honored as the default.
+        _noTemplating: true,


polar.angularaxis.type does not inherit from type in cartesian/layout_attributes.js. Should it also have _noTemplating: true?

good call - angularaxis.type gets _noTemplating in b1c6f0a

etpinard · 2018-07-03T15:39:46Z

test/jasmine/tests/template_test.js

+        1);
+    });
+
+    it('catches missing template.data', function() {


🐄 catches missing template.layout.

good catch! 818cac9

etpinard · 2018-07-03T16:43:42Z

Fantastic. 💃

alexcjohnson and others added 12 commits June 27, 2018 17:11

fix devtools timeit sorting

b8950c5

stop pushing tickmode back to axis in for a particular edge case

355fb09

minor simplification in pie defaults

8ddcfd6

combine annotation defaults files & shape defaults files

b5ccfbe

add visible attribute to lots of container array items:

86e09bb

rangeselector.buttons updatemenus.buttons slider.steps axes.tickformatstops mapbox.layers

use handleArrayContainerDefaults for various array containers:

3dee25c

rangeselector.buttons updatemenus.buttons parcoords & splom.dimensions (and DRY these a bit)

templates - big commit implementing most of it, coerce-level integration

5cca8d3

🔪 itemIsNotPlainObject from handleArrayContainerDefaults

b3da4e7

add template attributes to array containers

8525953

template-safe axis default color inheritance logic

e306d1c

there may be other similar logic we need to update for templates, but these are the most readily apparent ones

template-safe GUI editing of array objects

8e2a321

template mock

4346611

alexcjohnson added type: feature type: maintenance status: has TODOs labels Jun 27, 2018

etpinard added this to the v1.39.0 milestone Jun 27, 2018

etpinard reviewed Jun 27, 2018

View reviewed changes

alexcjohnson added 6 commits June 27, 2018 22:29

tickformatstops.visible -> enabled

fb489aa

🔪 done TODO

bc21cc8

fix test failures - and simplify handleArrayContainerDefaults even mo…

8490804

…re 🎉

fix makeTemplate and test it & template interactions

c2bcfe3

fix Plotly.validate with attributes that end in numbers

890a324

Plotly.validateTemplate

aed44dc

alexcjohnson added status: reviewable and removed status: has TODOs labels Jul 3, 2018

alexcjohnson commented Jul 3, 2018

View reviewed changes

loosen template default item interaction tests to pass on CI

6df61e0

alexcjohnson changed the title ~~[WIP] Templates~~ Templates Jul 3, 2018

TODO -> note re: axis.type _noTemplate

ef4c3cc

etpinard reviewed Jul 3, 2018

View reviewed changes

alexcjohnson added 4 commits July 3, 2018 18:15

_noTemplating for angularaxis.type

b1c6f0a

🐄 test name typo

818cac9

recurse into (template)layout looking for unused containers

15931cf

🔪 obsolete code

8598bc9

alexcjohnson merged commit fc0a1f8 into master Jul 3, 2018

alexcjohnson deleted the plotlyjs_templating branch July 3, 2018 16:48

etpinard mentioned this pull request Jul 4, 2018

paper_bgcolor and plot_bgcolor validation throws exception #2774

Closed

jonmmease mentioned this pull request Jul 19, 2018

WIP: adding themes plotly/plotly.py#924

Closed

3 tasks

etpinard mentioned this pull request Jul 23, 2018

plotly.js theme / style extraction and application #2469

Closed

alexcjohnson mentioned this pull request Jul 31, 2018

Parallel Categories trace type for multi dimensional categorical data jonmmease/plotly.js#1

Closed

4 tasks

chriddyp mentioned this pull request Sep 6, 2018

Introducing Templates/Themes plotly/plotly.py#425

Closed

jonmmease mentioned this pull request Sep 6, 2018

Improving integration of Plotly.js templates (themes) plotly/plotly.py#1161

Closed

fsmeier mentioned this pull request Sep 20, 2018

Template: options seem not to work (and/or missing documentation) #3032

Closed

jonmmease mentioned this pull request Oct 14, 2018

Templates (themes) integration plotly/plotly.py#1224

Merged

alexcjohnson mentioned this pull request Mar 23, 2020

Set Cartesian axis attributes via template #4670

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Templates #2761

Templates #2761

alexcjohnson commented Jun 27, 2018 •

edited

etpinard commented Jun 27, 2018

etpinard left a comment

etpinard Jun 27, 2018

etpinard Jun 27, 2018

alexcjohnson Jun 28, 2018

alexcjohnson Jun 28, 2018

etpinard Jun 28, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

etpinard Jun 27, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018 •

edited

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard Jul 3, 2018

alexcjohnson Jul 3, 2018

etpinard commented Jul 3, 2018

		var handleArrayContainerDefaults = require('../../plots/array_container_defaults');
		var handleAnnotationDefaults = require('./annotation_defaults');

Templates #2761

Templates #2761

Conversation

alexcjohnson commented Jun 27, 2018 • edited

etpinard commented Jun 27, 2018

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Jul 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Jul 3, 2018

alexcjohnson commented Jun 27, 2018 •

edited

etpinard Jul 3, 2018 •

edited