Add visible attribute to layout container items.
#1110
Conversation
- so that all array containers (not just gd.data) can use it.
- previously all relayout calls with leading `annotations` or `shapes` and value `null` were replaced with value `'remove'` - now make sure that only `annotations/shapes[i]: null` are replaced with `'remove'`
- so that the visibility of a given annotations can be turned off w/o having to remove to item or (in a hack) set 'opacity' to 0.
|
TODO:
|
- ... and make sure that
layout.images.length === fullLayout.image.length always,
i.e. item with no source are coerced to { visible: false }
and skipped during the draw step.
| @@ -17,6 +17,7 @@ | |||
| {"text":"right bottom","showarrow":false,"xref":"paper","yref":"paper","xanchor":"right","yanchor":"bottom","x":0.5,"y":1}, | |||
| {"text":"move with page","xref":"paper","yref":"paper","x":0.75,"y":1}, | |||
| {"text":"opacity","opacity":0.5,"x":5,"y":5}, | |||
| {"text":"not-visible", "visible": false}, | |||
There was a problem hiding this comment.
This mock is not autoranged... do you want to also include an invisible annotation in annotations-autorange.json, positioned to verify that it doesn't contribute? Same for shapes.
I guess images currently don't contribute to autorange at all - is that on purpose or should I consider it a bug?
There was a problem hiding this comment.
do you want to also include an invisible annotation in annotations-autorange.json
done in a2dd634
Same for shapes.
the shape_below_traces is autoranged, so that's a ✅ already
I guess images currently don't contribute to autorange at all - is that on purpose or should I consider it a bug?
I'd say that's a 🐛
There was a problem hiding this comment.
the
shape_below_tracesis autoranged, so that's a ✅ already
but the shape you added there just says {visible: false} so it will get auto coordinates too, which are going to be within the range chosen by autorange anyway, ie including it in autorange wouldn't make any difference.
There was a problem hiding this comment.
(actually that's true of a2dd634 too right?)
There was a problem hiding this comment.
@alexcjohnson good 👁️
I chose to revert a2dd634 and test annotations and shapes autorange using jasmine tests in 5e62c08
There was a problem hiding this comment.
Awesome, the new tests look great. Definitely better to do this with jasmine and relayout.
| return Plotly.relayout(gd, 'shapes[' + index + ']', 'remove'); | ||
| }) | ||
| .then(function() { | ||
| expect(countShapePathsInUpperLayer()).toEqual(pathCount); | ||
| expect(countShapes(gd)).toEqual(index); | ||
|
|
||
| return Plotly.relayout(gd, 'shapes[' + 1 + ']', null); | ||
| return Plotly.relayout(gd, 'shapes[' + 2 + '].visible', false); |
There was a problem hiding this comment.
'shapes[2].visible'? I guess this is to match 'shapes[' + index + ']' above but it looks a little funny...
|
|
||
| return Plotly.relayout(gd, 'images[1].visible', true); | ||
| }) | ||
| .then(function() { |
|
Really nice generalization of the |
| // xaxis2 need a bit more tolerance to pass on CI | ||
| // this most likely due to the different text bounding box values | ||
| // on headfull vs headless browsers. | ||
| var PREC2 = 0.1; |
There was a problem hiding this comment.
I'm confused - isn't this less tolerance?
There was a problem hiding this comment.
Ah I guess precision is supposed to be a number of digits, like the built-in toBeCloseTo... in which case I guess coercePosition shouldn't really treat 0 as special, 0 is just +/- 0.5... but that's an issue for another time. 💃
There was a problem hiding this comment.
Ah I guess precision is supposed to be a number of digits, like the built-in toBeCloseTo.
Exactly. Maybe we should add an array version of toBeWithin down the road too.
that is
annotations,shapesandimagesitems should get avisibleattribute just likeupdatemenusandslideralready have.This should make setting mobile-friendly annotations-less frames much easier. Related: #1081 (comment)
cc @rreusser @alexcjohnson