Add shapes.layer "between" for drawing between gridlines and traces #6927

my-tien · 2024-03-15T09:47:49Z

This PR adds the value "belowtraces" to the shapes.layer property. "below" draws the shape below the gridlines, "above" draws it above traces and "belowtraces" draws it between gridlines and traces.

I am required to add that…

the software is provided "as is", without warranty of any kind, express or implied, including but not limited to the warranties of merchantability, fitness for a particular purpose and noninfringement. in no event shall the authors or copyright holders be liable for any claim, damages or other liability, whether in an action of contract, tort or otherwise, arising from, out of or in connection with the software or the use or other dealings in the software.

src/components/shapes/attributes.js

archmoj · 2024-03-18T14:25:24Z

This option should be added to newshape too here:

plotly.js/src/components/shapes/draw_newshape/attributes.js

Line 120 in 08f21f0

values: ['below', 'above'],

That way one could use this feature when drawing new shapes.

my-tien · 2024-03-18T16:40:34Z

Hi @archmoj, thx for your feedback so far!
Please let me know if there are more locations I need to update for newshape. I didn't find any.

src/components/shapes/attributes.js

src/components/shapes/draw_newshape/attributes.js

my-tien · 2024-03-20T12:15:39Z

Hi, any pointers for this error that suddenly appeared? Can it be ignored?

@gl should fetch GeoJSON using URLs found in the traces
Test mapbox GeoJSON fetching:
Error: Expected Error: GeoJSON at URL "invalidUrl-two" does not exist. to equal Error: GeoJSON at URL "invalidUrl" does not exist..
at
at eval (webpack://plotly.js/./test/jasmine/tests/mapbox_test.js?:1800:22)
Error: Expected 'pending' to be undefined.
at
at eval (webpack://plotly.js/./test/jasmine/tests/mapbox_test.js?:1801:49)

archmoj · 2024-03-20T20:37:00Z

We fixed a problem on CircleCI (#6935).
Please pull upstream/master and merge it into this branch.

…idlines and traces

Make line length of description shorter.

archmoj · 2024-03-21T14:42:21Z

@my-tien Thanks very much for the PR.

Just a little note:
Force pushing to PRs is discouraged as described here: https://github.com/plotly/plotly.js/blob/master/.github/PULL_REQUEST_TEMPLATE.md

It's just fine for now. But for future work it would be better to pull upstream/master and merge it instead of rebasing and force push.

Thanks again for all your great contributions.

I would request a second review from @emilykl on this PR.

my-tien · 2024-03-21T15:08:43Z

@my-tien Thanks very much for the PR.

Just a little note: Force pushing to PRs is discouraged as described here: https://github.com/plotly/plotly.js/blob/master/.github/PULL_REQUEST_TEMPLATE.md

It's just fine for now. But for future work it would be better to pull upstream/master and merge it instead of rebasing and force push.

Thanks again for all your great contributions.

I would request a second review from @emilykl on this PR.

Right, I might have tested your tolerance there… will pay attention to that next time! :)

test/image/mocks/zz-shapes_layer_below_traces.json

src/components/shapes/draw.js

emilykl · 2024-03-22T19:53:13Z

How does this behave with multiple overlaid sets of axes and gridlines?

- added axis y2 to test interaction between axes - added shape label - moved dashdot line to see more of its interaction with the gridlines - removed transparency for easier visual checking

my-tien · 2024-03-25T12:18:10Z

How does this behave with multiple overlaid sets of axes and gridlines?

I have added a y2 axis to the test to verify that it looks correct

test/image/mocks/zz-shapes_layer_below_traces.json

src/components/shapes/attributes.js

draftlogs/6927_add.md

archmoj · 2024-03-26T15:50:05Z

@my-tien Please test the following mock:
It looks there is a problem placing some of the shapes in the relevant layer.

{
    "data":[{
        "x":[0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1],
        "y":[1,2,3,4,5,6,7,8,9,10],
        "mode":"markers"
    },
    {
        "x":["2000-01-01", "2000-01-02"],
        "y":["a", "b"],
        "mode":"markers",
        "xaxis":"x2",
        "yaxis":"y2"
    }],
    "layout": {
        "xaxis":{"title":{"text":"linear"},"range":[0,1],"domain":[0.35,0.65],"type":"linear","zeroline":false,"showticklabels":false},
        "yaxis":{"title":{"text":"log"},"range":[0,1],"domain":[0.6,1],"type":"log","zeroline":false,"showticklabels":false},
        "xaxis2":{"title":{"text":"date"},"range":["2000-01-01","2000-01-02"],"domain":[0.7,1],"anchor":"y2","type":"date","zeroline":false,"showticklabels":false},
        "yaxis2":{"title":{"text":"category"},"range":[0,1],"domain":[0.6,1],"anchor":"x2","type":"category","zeroline":false,"showticklabels":false},
        "height":400,
        "width":800,
        "margin": {"l":20,"r":20,"pad":0},
        "showlegend":false,
        "shapes":[
            {"layer":"between","x0":0.1,"x1":0.4,"y0":1.5,"y1":20,"opacity":0.5,"fillcolor":"#f00","line":{"width":8,"color":"#008","dash":"dashdot"}},
            {"layer":"between","path":"M0.5,3C0.5,9 0.9,9 0.9,3C0.9,1 0.5,1 0.5,3ZM0.6,4C0.6,5 0.66,5 0.66,4ZM0.74,4C0.74,5 0.8,5 0.8,4ZM0.6,3C0.63,2 0.77,2 0.8,3Z","fillcolor":"#fd2","line":{"width":1,"color":"black"}},
            {"layer":"between","xref":"x2","yref":"y2","type":"circle","x0":"2000-01-01 02","x1":"2000-01-01 08:30:33.456","y0":0.1,"y1":0.9,"fillcolor":"rgba(0,0,0,0.5)","line":{"color":"rgba(0,255,0,0.5)", "width":5}},
            {"layer":"between","xref":"x2","yref":"y2","path":"M2000-01-01_11:20:45.6,0.2Q2000-01-01_10:00,0.85 2000-01-01_21,0.8Q2000-01-01_22:20,0.15 2000-01-01_11:20:45.6,0.2Z","fillcolor":"rgb(151,73,58)"},
            {"layer":"between","xref":"x2","yref":"y2","type":"line","x0":"2000-01-01 11:00","x1":"2000-01-01 09:00","y0":"b","y1":"a","line":{"color":"#006","width":3}}
        ]
    }
}

my-tien · 2024-03-27T12:44:39Z

@my-tien Please test the following mock: It looks there is a problem placing some of the shapes in the relevant layer.

{
    "data":[{
        "x":[0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1],
        "y":[1,2,3,4,5,6,7,8,9,10],
        "mode":"markers"
    },
    {
        "x":["2000-01-01", "2000-01-02"],
        "y":["a", "b"],
        "mode":"markers",
        "xaxis":"x2",
        "yaxis":"y2"
    }],
    "layout": {
        "xaxis":{"title":{"text":"linear"},"range":[0,1],"domain":[0.35,0.65],"type":"linear","zeroline":false,"showticklabels":false},
        "yaxis":{"title":{"text":"log"},"range":[0,1],"domain":[0.6,1],"type":"log","zeroline":false,"showticklabels":false},
        "xaxis2":{"title":{"text":"date"},"range":["2000-01-01","2000-01-02"],"domain":[0.7,1],"anchor":"y2","type":"date","zeroline":false,"showticklabels":false},
        "yaxis2":{"title":{"text":"category"},"range":[0,1],"domain":[0.6,1],"anchor":"x2","type":"category","zeroline":false,"showticklabels":false},
        "height":400,
        "width":800,
        "margin": {"l":20,"r":20,"pad":0},
        "showlegend":false,
        "shapes":[
            {"layer":"between","x0":0.1,"x1":0.4,"y0":1.5,"y1":20,"opacity":0.5,"fillcolor":"#f00","line":{"width":8,"color":"#008","dash":"dashdot"}},
            {"layer":"between","path":"M0.5,3C0.5,9 0.9,9 0.9,3C0.9,1 0.5,1 0.5,3ZM0.6,4C0.6,5 0.66,5 0.66,4ZM0.74,4C0.74,5 0.8,5 0.8,4ZM0.6,3C0.63,2 0.77,2 0.8,3Z","fillcolor":"#fd2","line":{"width":1,"color":"black"}},
            {"layer":"between","xref":"x2","yref":"y2","type":"circle","x0":"2000-01-01 02","x1":"2000-01-01 08:30:33.456","y0":0.1,"y1":0.9,"fillcolor":"rgba(0,0,0,0.5)","line":{"color":"rgba(0,255,0,0.5)", "width":5}},
            {"layer":"between","xref":"x2","yref":"y2","path":"M2000-01-01_11:20:45.6,0.2Q2000-01-01_10:00,0.85 2000-01-01_21,0.8Q2000-01-01_22:20,0.15 2000-01-01_11:20:45.6,0.2Z","fillcolor":"rgb(151,73,58)"},
            {"layer":"between","xref":"x2","yref":"y2","type":"line","x0":"2000-01-01 11:00","x1":"2000-01-01 09:00","y0":"b","y1":"a","line":{"color":"#006","width":3}}
        ]
    }
}

Can you specify which layer is placed incorrectly? I couldn't find an issue with this example (I removed transparency and made the traces overlap all shapes to see that it correctly draws on top of the shapes):

{
  "data":[{
      "x":[0,0.1,0.2,0.3,0.4,0.5,0.6,0.7,0.8,0.9,1],
      "y":[1,2,3,4,5,6,6,6,6,6],
      "mode":"markers"
  },
  {
      "x":["2000-01-01", "2000-01-02"],
      "y":["a", "b"],
      "mode":"lines+markers",
      "xaxis":"x2",
      "yaxis":"y2"
  }],
  "layout": {
      "xaxis":{"title":{"text":"linear"},"range":[0,1],"domain":[0.35,0.65],"type":"linear","zeroline":false,"showticklabels":false},
      "yaxis":{"title":{"text":"log"},"range":[0,1],"domain":[0.6,1],"type":"log","zeroline":false,"showticklabels":false},
      "xaxis2":{"title":{"text":"date"},"range":["2000-01-01","2000-01-02"],"domain":[0.7,1],"anchor":"y2","type":"date","zeroline":false,"showticklabels":false},
      "yaxis2":{"title":{"text":"category"},"range":[0,1],"domain":[0.6,1],"anchor":"x2","type":"category","zeroline":false,"showticklabels":false},
      "height":400,
      "width":800,
      "margin": {"l":20,"r":20,"pad":0},
      "showlegend":false,
      "shapes":[
          {"layer":"between","x0":0.1,"x1":0.4,"y0":1.5,"y1":20,"opacity":1,"fillcolor":"#f00","line":{"width":8,"color":"#008","dash":"dashdot"}},
          {"layer":"between","path":"M0.5,3C0.5,9 0.9,9 0.9,3C0.9,1 0.5,1 0.5,3ZM0.6,4C0.6,5 0.66,5 0.66,4ZM0.74,4C0.74,5 0.8,5 0.8,4ZM0.6,3C0.63,2 0.77,2 0.8,3Z","fillcolor":"#fd2","line":{"width":1,"color":"black"}},
          {"layer":"between","xref":"x2","yref":"y2","type":"circle","x0":"2000-01-01 02","x1":"2000-01-01 08:30:33.456","y0":0.1,"y1":0.9,"fillcolor":"rgba(0,0,0,1)","line":{"color":"rgba(0,255,0,1)", "width":5}},
          {"layer":"between","xref":"x2","yref":"y2","path":"M2000-01-01_11:20:45.6,0.2Q2000-01-01_10:00,0.85 2000-01-01_21,0.8Q2000-01-01_22:20,0.15 2000-01-01_11:20:45.6,0.2Z","fillcolor":"rgb(151,73,58)"},
          {"layer":"between","xref":"x2","yref":"y2","type":"line","x0":"2000-01-01 11:00","x1":"2000-01-01 09:00","y0":"b","y1":"a","line":{"color":"#006","width":3}}
      ]
  }
}

…inct

archmoj · 2024-03-27T13:26:26Z

Thanks for testing it out. So It was because of the transparency that I thought there is an issue.
Resolved.

@my-tien Please test the following mock: It looks there is a problem placing some of the shapes in the relevant layer.

Can you specify which layer is placed incorrectly? I couldn't find an issue with this example (I removed transparency and made the traces overlap all shapes to see that it correctly draws on top of the shapes):

archmoj · 2024-03-28T13:10:28Z

Nicely completed.
💃

my-tien force-pushed the below_traces branch 2 times, most recently from 16b6139 to 7338ad8 Compare March 15, 2024 16:09

archmoj added feature something new community community contribution status: reviewable labels Mar 15, 2024

archmoj reviewed Mar 15, 2024

View reviewed changes

src/components/shapes/attributes.js Outdated Show resolved Hide resolved

my-tien force-pushed the below_traces branch from 0861ab6 to 20b58aa Compare March 18, 2024 07:30

archmoj changed the title ~~Add shapes.layer "belowtraces" for drawing between gridlines and traces~~ Add shapes.layer "between" for drawing between gridlines and traces Mar 18, 2024

archmoj reviewed Mar 18, 2024

View reviewed changes

src/components/shapes/attributes.js Outdated Show resolved Hide resolved

archmoj reviewed Mar 18, 2024

View reviewed changes

src/components/shapes/draw_newshape/attributes.js Outdated Show resolved Hide resolved

tarzzz assigned archmoj Mar 20, 2024

my-tien added 8 commits March 21, 2024 09:41

Add shapes.layer value "belowtraces" for drawing shapes in between gr…

9bcf7e6

…idlines and traces

Add draftlog

0f9da5a

Add baseline image for shapes_layer_below_traces.json

267867b

Make sure new test images run last

b25862d

Update splom_test's innerSubplotNodeCnt and index of xaxislayer-above

7a7e57f

Rename "belowtraces" to "between"

2b398d3

Add "between to newshape.layer as well.

70ebe72

Make line length of description shorter.

Fix descriptions for shapes.layer and newshape.layer

11e05a1

my-tien force-pushed the below_traces branch from af7fa14 to 11e05a1 Compare March 21, 2024 13:19

archmoj requested a review from emilykl March 21, 2024 14:42

emilykl reviewed Mar 22, 2024

View reviewed changes

test/image/mocks/zz-shapes_layer_below_traces.json Outdated Show resolved Hide resolved

emilykl reviewed Mar 22, 2024

View reviewed changes

src/components/shapes/draw.js Show resolved Hide resolved

Test more cases in shapes_layer_below_traces mock

ff7ca7a

- added axis y2 to test interaction between axes - added shape label - moved dashdot line to see more of its interaction with the gridlines - removed transparency for easier visual checking

Updated baseline image for changed shapes_layer_below_traces mock

410f7cf