-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Add layout.shapes.layer
#439
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
Add layout.shapes.layer
#439
Conversation
This PR is ready for review. |
Seems reasonable to me - we generally like to put have a bit of discussion when adding attributes - thoughts on this @etpinard? I still have a few things I'd like to check on this, but it looks good so far. Maybe take a look into resolving whatever the conflict is and create an image test for the two functionalities as well - would be useful for catching future shapelayer bugs. |
* Added functionality to show layer below traces. * The axis references of a 'below'-shape determine the subplots under which a shape is shown. * If both axis references of a 'below'-shape are set to 'paper', then the shape is shown below all the subplots. * Updated `test/image/mocks/shapes.json` to exercise the new functionality. * Updated `test/jasmine/tests/shapes_test.js` to account for the new shape layers.
96657aa
to
a8aa194
Compare
@n-riesco Looks like you're on the right track.
Agreed. 👍 . But I found one bug. See below comment.
You mean if |
Consider: var y = [1,2,1,0,-1,2,3,5];
Plotly.plot(Tabs.fresh(), [
{
y: y,
line: { shape: 'spline' },
}, {
y: y.map((v) => 10 * Math.sin(Math.PI * v / 4)),
line: { shape: 'spline' },
xaxis: 'x2'
}, {
y: y.map((v) => 10 * Math.cos(Math.PI * v / 4)),
line: { shape: 'spline' },
yaxis: 'y2'
}, {
y: y.map((v) => 10 / (v + 4)),
line: { shape: 'spline' },
xaxis: 'x2',
yaxis: 'y2'
}],
{
title: 'shape shading a region',
showlegend: false,
xaxis: {
domain: [0, 0.45]
},
xaxis2: {
domain: [0.55, 1]
},
yaxis: {
domain: [0, 0.45]
},
yaxis2: {
domain: [0.55, 1]
},
shapes: [
{
type: 'rect',
layer: 'below',
xref: 'x',
x0: 3.5,
x1: 4.5,
yref: 'paper',
y0: 0,
y1: 1,
fillcolor: '#c7eae5',
},
{
type: 'rect',
layer: 'above',
xref: 'x2',
x0: 5.5,
x1: 6.5,
yref: 'paper',
y0: 0,
y1: 1,
fillcolor: '#c7eae5',
opacity: 0.5
},
{
type: 'rect',
layer: 'below',
xref: 'paper',
x0: 0,
x1: 1,
yref: 'y',
y0: 0,
y1: 3,
fillcolor: '#f6e8c3',
},
{
type: 'rect',
layer: 'above',
xref: 'paper',
x0: 0,
x1: 1,
yref: 'y2',
y0: 1,
y1: 4,
fillcolor: '#f6e8c3',
opacity: 0.5
},
{
type: 'rect',
layer: 'below',
xref: 'paper',
x0: 0.3,
x1: 0.7,
yref: 'paper',
y0: 0.3,
y1: 0.7,
fillcolor: '#d3d3d3',
}
],
dragmode: 'pan'
}
) This gives where both the |
TODOs:
|
}); | ||
} | ||
|
||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐄 no need for this. Save the bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it.
I usually write closures at the end of a function and use the return
to indicate that there are no more statements hidden after the closures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand. I'm not saying that it's a bad habit. It's really a matter of consistency with the rest of the source code.
* Fixed bug that caused shapes drawn in a subplot to extend beyond the limits of the subplot. * Fixed issues with code styling.
* Added test that updates the layer property of a shape.
* Added a mock the places shapes in lower, upper and subplot shape layers. * The shapes extend over several subplots to test whether the appropriate clip paths have been set.
@etpinard I've addressed all the points in the TODO list and fixed a bug that prevented shape properties from being updated. I've added your plot to the image mocks, because it nicely exercises all the new functionality, i.e.:
|
.toEqual(shapesInUpperLayer + 1); | ||
expect(getLastShape(gd)).toEqual(shape); | ||
expect(countShapes(gd)).toEqual(index + 1); | ||
}).then(done); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nicely done
@n-riesco great PR! 🍻 |
I've got a question: When using shapes, if I set layer |
test/image/mocks/shapes.json
to exercise the new functionality.test/jasmine/tests/shapes_test.js
to account for the new shape layers.Closes #148