Skip to content

restyle/relayout refactor #1999

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 43 commits into from
Sep 20, 2017
Merged

restyle/relayout refactor #1999

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
43 commits
Select commit Hold shift + click to select a range
4d3e2ec
test - and fix - most of the relayout doextras
alexcjohnson Aug 18, 2017
95d7d71
test - and fix - most of the doextra calls in restyle
alexcjohnson Aug 29, 2017
3326ecc
test that xaxis-only items are only in the xaxis in the schema
alexcjohnson Aug 30, 2017
a948cce
merge component attribute schemas into core at register time
alexcjohnson Aug 30, 2017
dd52922
fix lib test for undefined -> null in undoqueue
alexcjohnson Sep 1, 2017
f90f079
abstract id/name counter regex and standardize cartesian attrRegex
alexcjohnson Sep 1, 2017
e036fea
fix #1325 - animating multiple axes
alexcjohnson Sep 1, 2017
69e0188
Plotschema getTraceValObject and getLayoutValObject methods
alexcjohnson Sep 1, 2017
bbfe399
relativeAttr
alexcjohnson Sep 12, 2017
29931ec
fix annotation comments/descriptions
alexcjohnson Sep 12, 2017
fad72a2
make common hover label pick up changes quicker
alexcjohnson Sep 13, 2017
e895b32
edit_types.overrideAll
alexcjohnson Sep 13, 2017
7a7dc6d
let PlotSchema.crawl report the complete attribute string
alexcjohnson Sep 13, 2017
c87b01a
better reporting from hover label test
alexcjohnson Sep 13, 2017
658e5cb
fix registry for new circular dep
alexcjohnson Sep 13, 2017
f49ae5e
massive commit to lock in editType and impliedEdits and clean up rest…
alexcjohnson Sep 13, 2017
7ea0d25
lint
alexcjohnson Sep 13, 2017
284c87f
remove obsolete comment in gl3d
alexcjohnson Sep 14, 2017
b9826c8
change overrideAll API to nested/from-root only
alexcjohnson Sep 14, 2017
7c38a4a
clean up restyle/relayout flag names
alexcjohnson Sep 14, 2017
96cc57f
clean up editTypes/impliedEdits and formalize & document their schema…
alexcjohnson Sep 14, 2017
cb94e95
test restrictions on component xaxis/yaxis schemas
alexcjohnson Sep 15, 2017
238e248
preserve impliedEdits: {key: undefined} by extendDeepAll
alexcjohnson Sep 15, 2017
42662ba
comments on relative_attr regexps
alexcjohnson Sep 15, 2017
50aa1ca
include schema in dist
alexcjohnson Sep 15, 2017
62a1392
fix plotschema test for metaKeys
alexcjohnson Sep 15, 2017
87b26d5
test order-independence of trace/transform/component registration
alexcjohnson Sep 18, 2017
040ed1b
test colorbar editing
alexcjohnson Sep 19, 2017
388a7fe
abstract - and fix - automatic axis type clearing
alexcjohnson Sep 19, 2017
6e8a68c
coerceTraceIndices earlier so clearAxisTypes can use it
alexcjohnson Sep 19, 2017
4f8fc66
move clearAxisTypes into helpers
alexcjohnson Sep 19, 2017
97ddf48
update jsdom to v11.2 with new API
alexcjohnson Sep 19, 2017
fe7db79
oops didn't mean to commit that commented out...
alexcjohnson Sep 19, 2017
68f5dbc
fix and test errorbar visibility toggling
alexcjohnson Sep 19, 2017
d15e541
layout.showlegend test
alexcjohnson Sep 19, 2017
7ec1634
closes #615 - something else in this PR fixed it, just nailing a test
alexcjohnson Sep 19, 2017
a107466
fix #358 - restyling orientation
alexcjohnson Sep 20, 2017
42805c2
test histogram changing data type
alexcjohnson Sep 20, 2017
8d9feaf
fix #2020 - editing plots with multiple histograms
alexcjohnson Sep 20, 2017
0a98a6d
lint
alexcjohnson Sep 20, 2017
e4227aa
move checkTicks into custom_assertions
alexcjohnson Sep 20, 2017
0729921
load custom_matchers globally, and refactor negateIf as a method
alexcjohnson Sep 20, 2017
407ae5a
pull custom_matchers out of requirejs bundle test
alexcjohnson Sep 20, 2017
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
15 changes: 7 additions & 8 deletions src/components/annotations/arrow_paths.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,13 @@
'use strict';

/**
* centerx is a center of scaling tuned for maximum scalability of
* the arrowhead ie throughout mag=0.3..3 the head is joined smoothly
* to the line, but the endpoint moves.
* backoff is the distance to move the arrowhead, and the end of the
* line, in order to end at the right place
*
* TODO: option to have the pointed-to point a little in front of the
* end of the line, as people tend to want a bit of a gap there...
* All paths are tuned for maximum scalability of the arrowhead,
* ie throughout arrowwidth=0.3..3 the head is joined smoothly
* to the line, with the line coming from the left and ending at (0, 0).
* `backoff` is the distance to move the arrowhead and the end of the line,
* in order that the arrowhead points to the desired place, either at
* the tip of the arrow or (in the case of circle or square)
* the center of the symbol.
*/

module.exports = [
Expand Down
57 changes: 51 additions & 6 deletions src/components/annotations/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
var ARROWPATHS = require('./arrow_paths');
var fontAttrs = require('../../plots/font_attributes');
var cartesianConstants = require('../../plots/cartesian/constants');
var extendFlat = require('../../lib/extend').extendFlat;


module.exports = {
Expand All @@ -21,6 +20,7 @@ module.exports = {
valType: 'boolean',
role: 'info',
dflt: true,
editType: 'docalcAutorange',
Copy link
Contributor

Choose a reason for hiding this comment

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

Would docalc+autorange be a sensible name for this? It sounds like an enum camelcased and concatenated, but I could be mistaken.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. Unless I'm mistaken, this adds no new labels. That's just plugging into what already exists. Still seems like maybe an enum in disguise, but nothing to change here then.

Copy link
Contributor

Choose a reason for hiding this comment

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

One additional note while digging through here is that the do- prefix seems to be carried forward from what was originally just internal variable names. Would editType: 'calcAutorange' or editType: 'calc+autorange' make sense? It seems a bit friendlier if you're trying to make sense of the schema as a standalone concept.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

docalcAutorange is a single thing, means "do a recalc only if there's an autoranged axis related to this attribute" - Actually, ideally we'd specify what to do if there isn't an autoranged axis... so like docalcAutorange+dostyle or something.

Would editType: 'calcAutorange' make sense?

Yeah totally, good idea! And to help us out (to figure out what editType a new attribute should get) I should probably describe somewhere what these flags actually mean in terms of the code paths they lead to.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe calcIfAutorange?

to figure out what editType a new attribute should get

FWIW, I have a history of making bad choices here for lack of really understanding the implications of some schema concepts. If you want I can try making docs/schema.md that explains, at the very least, what some of the schema items link up with. Some of the concepts like _isLinkedToArray get a little subtle. Even for the fifth time interacting with it, I think a brief description with links to a couple relevant spots in the code would help.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe calcIfAutorange?

🏆

If you want I can try making docs/schema.md

Definitely time for something like that! @etpinard thoughts about how/where to do this? Should it be a standalone thing, or somehow included in Plotly.PlotSchema.get() or...?

Copy link
Contributor

Choose a reason for hiding this comment

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

My vote would be for not including in the code so that it's as accessible and readable as possible without having to realize it's an API function and instantiate anything in order to access.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd vote for adding a attribute-like edit type declaration object in edit_types.js with a description field. For example,

var traceEditTypes: {
  docalc: {
    description: [
       // ....
    ].join(' ')
  },
  // ....
}

That description field would get stripped off by the compress_attributes transform. Plotly.PlotSchema.get().defs.editTypes would then return the list of edit types with their description.

I see your point @rreusser about relying an API function to show docs being less-than-ideal. But since we already have a hard time keeping https://github.com/plotly/documentation/ up to speed, adding another layer of static (e.g. a github wiki or a docs/ directory) documentation sounds dangerous. No docs is better than bad docs, right? That said, it wouldn't hurt adding a few lines in our CONTRIBUTING.md over the edit types.

description: [
'Determines whether or not this annotation is visible.'
].join(' ')
Expand All @@ -29,6 +29,7 @@ module.exports = {
text: {
valType: 'string',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the text associated with this annotation.',
'Plotly uses a subset of HTML tags to do things like',
Expand All @@ -41,19 +42,23 @@ module.exports = {
valType: 'angle',
dflt: 0,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets the angle at which the `text` is drawn',
'with respect to the horizontal.'
].join(' ')
},
font: extendFlat({}, fontAttrs, {
font: fontAttrs({
editType: 'docalcAutorange',
colorEditType: 'doarraydraw',
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you point me toward what doarraydraw does? It's not entirely clear from the name, and if it appears somewhere else in the source, it's not jumping out at me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind. Found it in the suppressed diff.

Copy link
Contributor

@etpinard etpinard Sep 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Wait, where is doarraydraw being used? I can't find it (even in the suppressed diffs 🙃 )

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 think at one point I had none instead of doarraydraw but that looked weird. Then at another point I had a block just for debugging that checked that everything labeled doarraydraw was captured by an arrayContainers block and threw an error otherwise, but I removed that once I was satisfied that it was never getting hit... happy to change it if you think it would make more sense another way.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly, doarraydraw is just a placeholder for edits in array containers that are handled by applyContainerArrayChanges in manage_array.js.

In this case, I prefer setting editType: 'doarraydraw' over editType: 'none'. Just make sure that this behavior is documented in the edit type description field (cc #1999 (comment)).

description: 'Sets the annotation text font.'
}),
width: {
valType: 'number',
min: 1,
dflt: null,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets an explicit width for the text box. null (default) lets the',
'text set the box width. Wider text will be clipped.',
Expand All @@ -65,6 +70,7 @@ module.exports = {
min: 1,
dflt: null,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets an explicit height for the text box. null (default) lets the',
'text set the box height. Taller text will be clipped.'
Expand All @@ -76,13 +82,15 @@ module.exports = {
max: 1,
dflt: 1,
role: 'style',
editType: 'doarraydraw',
description: 'Sets the opacity of the annotation (text + arrow).'
},
align: {
valType: 'enumerated',
values: ['left', 'center', 'right'],
dflt: 'center',
role: 'style',
editType: 'doarraydraw',
description: [
'Sets the horizontal alignment of the `text` within the box.',
'Has an effect only if `text` spans more two or more lines',
Expand All @@ -95,6 +103,7 @@ module.exports = {
values: ['top', 'middle', 'bottom'],
dflt: 'middle',
role: 'style',
editType: 'doarraydraw',
description: [
'Sets the vertical alignment of the `text` within the box.',
'Has an effect only if an explicit height is set to override',
Expand All @@ -105,12 +114,14 @@ module.exports = {
valType: 'color',
dflt: 'rgba(0,0,0,0)',
role: 'style',
editType: 'doarraydraw',
description: 'Sets the background color of the annotation.'
},
bordercolor: {
valType: 'color',
dflt: 'rgba(0,0,0,0)',
role: 'style',
editType: 'doarraydraw',
description: [
'Sets the color of the border enclosing the annotation `text`.'
].join(' ')
Expand All @@ -120,6 +131,7 @@ module.exports = {
min: 0,
dflt: 1,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets the padding (in px) between the `text`',
'and the enclosing border.'
Expand All @@ -130,6 +142,7 @@ module.exports = {
min: 0,
dflt: 1,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets the width (in px) of the border enclosing',
'the annotation `text`.'
Expand All @@ -140,6 +153,7 @@ module.exports = {
valType: 'boolean',
dflt: true,
role: 'style',
editType: 'docalcAutorange',
description: [
'Determines whether or not the annotation is drawn with an arrow.',
'If *true*, `text` is placed near the arrow\'s tail.',
Expand All @@ -149,6 +163,7 @@ module.exports = {
arrowcolor: {
valType: 'color',
role: 'style',
editType: 'doarraydraw',
description: 'Sets the color of the annotation arrow.'
},
arrowhead: {
Expand All @@ -157,26 +172,33 @@ module.exports = {
max: ARROWPATHS.length,
dflt: 1,
role: 'style',
editType: 'doarraydraw',
description: 'Sets the annotation arrow head style.'
},
arrowsize: {
valType: 'number',
min: 0.3,
dflt: 1,
role: 'style',
description: 'Sets the size (in px) of annotation arrow head.'
editType: 'docalcAutorange',
description: [
'Sets the size of the annotation arrow head, relative to `arrowwidth`.',
'A value of 1 (default) gives a head about 3x as wide as the line.'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks 📚

].join(' ')
},
arrowwidth: {
valType: 'number',
min: 0.1,
role: 'style',
description: 'Sets the width (in px) of annotation arrow.'
editType: 'docalcAutorange',
description: 'Sets the width (in px) of annotation arrow line.'
},
standoff: {
valType: 'number',
min: 0,
dflt: 0,
role: 'style',
editType: 'docalcAutorange',
description: [
'Sets a distance, in pixels, to move the arrowhead away from the',
'position it is pointing at, for example to point at the edge of',
Expand All @@ -188,6 +210,7 @@ module.exports = {
ax: {
valType: 'any',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the x component of the arrow tail about the arrow head.',
'If `axref` is `pixel`, a positive (negative) ',
Expand All @@ -200,6 +223,7 @@ module.exports = {
ay: {
valType: 'any',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the y component of the arrow tail about the arrow head.',
'If `ayref` is `pixel`, a positive (negative) ',
Expand All @@ -217,6 +241,7 @@ module.exports = {
cartesianConstants.idRegex.x.toString()
],
role: 'info',
editType: 'docalc',
description: [
'Indicates in what terms the tail of the annotation (ax,ay) ',
'is specified. If `pixel`, `ax` is a relative offset in pixels ',
Expand All @@ -234,6 +259,7 @@ module.exports = {
cartesianConstants.idRegex.y.toString()
],
role: 'info',
editType: 'docalc',
description: [
'Indicates in what terms the tail of the annotation (ax,ay) ',
'is specified. If `pixel`, `ay` is a relative offset in pixels ',
Expand All @@ -251,6 +277,7 @@ module.exports = {
cartesianConstants.idRegex.x.toString()
],
role: 'info',
editType: 'docalc',
description: [
'Sets the annotation\'s x coordinate axis.',
'If set to an x axis id (e.g. *x* or *x2*), the `x` position',
Expand All @@ -263,6 +290,7 @@ module.exports = {
x: {
valType: 'any',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the annotation\'s x position.',
'If the axis `type` is *log*, then you must take the',
Expand All @@ -280,6 +308,7 @@ module.exports = {
values: ['auto', 'left', 'center', 'right'],
dflt: 'auto',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the text box\'s horizontal position anchor',
'This anchor binds the `x` position to the *left*, *center*',
Expand All @@ -298,6 +327,7 @@ module.exports = {
valType: 'number',
dflt: 0,
role: 'style',
editType: 'docalcAutorange',
description: [
'Shifts the position of the whole annotation and arrow to the',
'right (positive) or left (negative) by this many pixels.'
Expand All @@ -310,6 +340,7 @@ module.exports = {
cartesianConstants.idRegex.y.toString()
],
role: 'info',
editType: 'docalc',
description: [
'Sets the annotation\'s y coordinate axis.',
'If set to an y axis id (e.g. *y* or *y2*), the `y` position',
Expand All @@ -322,6 +353,7 @@ module.exports = {
y: {
valType: 'any',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the annotation\'s y position.',
'If the axis `type` is *log*, then you must take the',
Expand All @@ -339,6 +371,7 @@ module.exports = {
values: ['auto', 'top', 'middle', 'bottom'],
dflt: 'auto',
role: 'info',
editType: 'docalcAutorange',
description: [
'Sets the text box\'s vertical position anchor',
'This anchor binds the `y` position to the *top*, *middle*',
Expand All @@ -357,6 +390,7 @@ module.exports = {
valType: 'number',
dflt: 0,
role: 'style',
editType: 'docalcAutorange',
description: [
'Shifts the position of the whole annotation and arrow up',
'(positive) or down (negative) by this many pixels.'
Expand All @@ -367,6 +401,7 @@ module.exports = {
values: [false, 'onoff', 'onout'],
dflt: false,
role: 'style',
editType: 'doarraydraw',
description: [
'Makes this annotation respond to clicks on the plot.',
'If you click a data point that exactly matches the `x` and `y`',
Expand All @@ -385,6 +420,7 @@ module.exports = {
xclick: {
valType: 'any',
role: 'info',
editType: 'doarraydraw',
description: [
'Toggle this annotation when clicking a data point whose `x` value',
'is `xclick` rather than the annotation\'s `x` value.'
Expand All @@ -393,6 +429,7 @@ module.exports = {
yclick: {
valType: 'any',
role: 'info',
editType: 'doarraydraw',
description: [
'Toggle this annotation when clicking a data point whose `y` value',
'is `yclick` rather than the annotation\'s `y` value.'
Expand All @@ -401,6 +438,7 @@ module.exports = {
hovertext: {
valType: 'string',
role: 'info',
editType: 'doarraydraw',
description: [
'Sets text to appear when hovering over this annotation.',
'If omitted or blank, no hover label will appear.'
Expand All @@ -410,6 +448,7 @@ module.exports = {
bgcolor: {
valType: 'color',
role: 'style',
editType: 'doarraydraw',
description: [
'Sets the background color of the hover label.',
'By default uses the annotation\'s `bgcolor` made opaque,',
Expand All @@ -419,23 +458,27 @@ module.exports = {
bordercolor: {
valType: 'color',
role: 'style',
editType: 'doarraydraw',
description: [
'Sets the border color of the hover label.',
'By default uses either dark grey or white, for maximum',
'contrast with `hoverlabel.bgcolor`.'
].join(' ')
},
font: extendFlat({}, fontAttrs, {
font: fontAttrs({
editType: 'doarraydraw',
description: [
'Sets the hover label text font.',
'By default uses the global hover font and size,',
'with color from `hoverlabel.bordercolor`.'
].join(' ')
})
}),
editType: 'doarraydraw'
},
captureevents: {
valType: 'boolean',
role: 'info',
editType: 'doarraydraw',
description: [
'Determines whether the annotation text box captures mouse move',
'and click events, or allows those events to pass through to data',
Expand All @@ -445,11 +488,13 @@ module.exports = {
'you must explicitly enable `captureevents`.'
].join(' ')
},
editType: 'docalc',

_deprecated: {
ref: {
valType: 'string',
role: 'info',
editType: 'docalc',
description: [
'Obsolete. Set `xref` and `yref` separately instead.'
].join(' ')
Expand Down
7 changes: 4 additions & 3 deletions src/components/annotations3d/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,9 @@
'use strict';

var annAtts = require('../annotations/attributes');
var overrideAll = require('../../plot_api/edit_types').overrideAll;

module.exports = {
module.exports = overrideAll({
_isLinkedToArray: 'annotation',

visible: annAtts.visible,
Expand Down Expand Up @@ -76,7 +77,7 @@ module.exports = {
standoff: annAtts.standoff,
hovertext: annAtts.hovertext,
hoverlabel: annAtts.hoverlabel,
captureevents: annAtts.captureevents
captureevents: annAtts.captureevents,

// maybes later?
// clicktoshow: annAtts.clicktoshow,
Expand All @@ -89,4 +90,4 @@ module.exports = {
// xref: 'x'
// yref: 'y
// zref: 'z'
};
}, 'docalc', true);
Loading