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

Add layout.shapes.layer #439

Merged

Conversation

n-riesco
Copy link
Contributor

@n-riesco n-riesco commented Apr 18, 2016

  • Added functionality to show shapers 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.

Closes #148

@n-riesco
Copy link
Contributor Author

This PR is ready for review.

@mdtusz
Copy link
Contributor

mdtusz commented Apr 18, 2016

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.
@n-riesco n-riesco force-pushed the issue-148-implement-shapes-below-traces branch from 96657aa to a8aa194 Compare April 18, 2016 14:59
@etpinard
Copy link
Contributor

@n-riesco Looks like you're on the right track.

The axis-references of a 'below'-shape determine the subplots under which a shape is shown.

Agreed. 👍 . But I found one bug. See below comment.

If both axis-references of a 'below'-shape are set to 'paper', then the shape is shown below all the subplots

You mean if xref and yref are set to 'paper' ? In that case, yep, I agree 👍

@etpinard
Copy link
Contributor

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

image

where both the xref: 'x' and the yref: 'y' shapes appear above the xy subplot.

@etpinard
Copy link
Contributor

etpinard commented Apr 18, 2016

TODOs:

});
}

return;
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.
@n-riesco
Copy link
Contributor Author

@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.:

  • places shapes in the lower, upper and subplot shape layers,
  • and uses shapes that extend across several subplots (to test the appropriate clip paths are used).

.toEqual(shapesInUpperLayer + 1);
expect(getLastShape(gd)).toEqual(shape);
expect(countShapes(gd)).toEqual(index + 1);
}).then(done);
Copy link
Contributor

Choose a reason for hiding this comment

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

nicely done

@etpinard
Copy link
Contributor

@n-riesco great PR! 🍻

@etpinard etpinard merged commit 1c84913 into plotly:master Apr 19, 2016
@n-riesco n-riesco deleted the issue-148-implement-shapes-below-traces branch April 19, 2016 16:30
@etpinard etpinard mentioned this pull request Sep 9, 2016
@rolandjitsu
Copy link

I've got a question: When using shapes, if I set layer below, the grid lines are above the layer, is this expected behaviour? If yes, is there a way to place the shapes under the traces but above the grid lines?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants