NotificationsYou must be signed in to change notification settings
Fork2k
Star18k

Property`zerolinelayer` for cartesian axes#7269

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

gvwilson merged 7 commits intoplotly:masterfrommy-tien:zeroline-layer

May 8, 2025

Merged

Property`zerolinelayer` for cartesian axes#7269

gvwilson merged 7 commits intoplotly:masterfrommy-tien:zeroline-layer

May 8, 2025

Conversation

Copy link

Contributor

my-tien commentedNov 13, 2024•
edited
Loading

Adds a new propertyzerolinelayer to cartesian axes to allow drawing the zeroline above traces.

This addresses the issue that sometimes the zeroline can be barely visible:

When setting the new propertyyaxis.zerolinelayer = "above traces":

Disclaimer

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.

my-tien added3 commits

November 13, 2024 15:59

New axis propzerolinelayer with values "below traces" (default) an…

dc58b26

…d "above traces"

baseline images for zerolinelayer mocks, draftlog

24d1082

Adjust splom test after addition of zerolinelayerAbove

2d94bef

my-tien mentioned this pull request

Nov 13, 2024

Add support for xref/yref paper for shapes with layer "between"#6989

Closed

my-tien changed the title~~New axis propzerolinelayer with values "below traces" (default) and "above traces"~~Propertyzerolinelayer for cartesian axes

Nov 13, 2024

gvwilson self-assigned this

Nov 13, 2024

gvwilson added feature

something new

communitycommunity contribution labels

Nov 13, 2024

Copy link

ContributorAuthor

my-tien commentedNov 13, 2024

@gvwilson Do you have a means to trigger the CI again? It seems that this run had a connection error on launch.

Copy link

Contributor

gvwilson commentedNov 13, 2024

@my-tien I just re-ran CI - hope that helps.

gvwilson added the cscustomer success label

Nov 21, 2024

Copy link

stephprobst commentedFeb 3, 2025•
edited
Loading

Hi@gvwilson - Could this PR be reviewed and possibly merged soon? It seems like a reasonably sized change, that should be easy to integrate? Thank you for considering!

gvwilson added the P1needed for current cycle label

Feb 3, 2025

gvwilson assignedemilykl and unassignedgvwilson

Feb 4, 2025

emilykl reviewed

Feb 11, 2025

View reviewed changes

src/plots/cartesian/line_grid_defaults.js Outdated

		varshowZeroLine=coerce('zeroline',opts.showGrid\|\|!!zeroLineColor\|\|!!zeroLineWidth);

		if(!showZeroLine){
		deletecontainerOut.zerolineLayer;

Copy link

Contributor

emilyklFeb 11, 2025

Choose a reason for hiding this comment

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

Suggested change

	deletecontainerOut.zerolineLayer;
	deletecontainerOut.zerolinelayer;

Copy link

ContributorAuthor

my-tienFeb 12, 2025

Choose a reason for hiding this comment

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

Thx for spotting! Fixed. Btw. did you just see this or is there another trick to catch this mistake?

Copy link

Contributor

emilykl commentedFeb 11, 2025•
edited
Loading

@my-tien Thank you for the contribution!

This is looking good to me -- however I've noticed thatzerolinelayer doesn't seem to play nicely with thezorder property -- when a trace on a plot haszorder set, the traces are rendered above the zerolines, even ifzerolinelayer is set toabove traces. I've included an example below.

How big of a problem is that on your side? We are open to simply documenting the limitation but want to verify whether it will cause issues for you.

Mock:

Details

{    "data": [        {            "type": "bar",            "x": [-1, 0, 2, 3],            "y": [-1, 2, -3, 4],            "name": "xy",            "zorder": 2        },        {            "line": {                "width": 10            },            "mode": "lines",            "x": [-1, 0, 2, 3],            "y": [-3, 1, -1, 2],            "xaxis": "x2",            "yaxis": "y2",            "name": "x2y2"        }    ],    "layout": {        "title": {            "text": "Zerolines should be above traces<br>but they are rendered below"        },        "width": 600,        "height": 400,        "xaxis": {            "showline": true,            "zeroline": true,            "zerolinewidth": 5,            "zerolinecolor": "pink",            "zerolinelayer": "above traces"        },        "xaxis2": {            "showline": true,            "overlaying": "x",            "zeroline": true,            "zerolinewidth": 5,            "zerolinecolor": "pink",            "zerolinelayer": "above traces",            "side": "top"        },        "yaxis": {            "showline": true,            "zeroline": true,            "zerolinewidth": 5,            "zerolinecolor": "black",            "zerolinelayer": "above traces",            "range": [-1, 4]        },        "yaxis2": {            "showline": true,            "zeroline": true,            "zerolinewidth": 5,            "zerolinecolor": "black",            "zerolinelayer": "above traces",            "range": [-3, 2],            "overlaying": "y",            "side": "right"        }    }}

Image:

my-tien added3 commits

February 12, 2025 11:18

Fix typo in line_grid_defaults zerolineLayer → zerolinelayer

1a41bb7

Merge remote-tracking branch 'origin-plotly/master' into zeroline-layer

d02912f

zerolinelayer description: Add note that it doesn't work in combinati…

eb2ead5

…on with zorder.

Copy link

ContributorAuthor

my-tien commentedFeb 12, 2025

Hi Emily, thanks for the heads up.

I didn't dig into the zorder logic much, but I assume to make it work with it, the zeroline drawing would have to be moved to another location? So it sounds like it could become a major change (correct me if I'm wrong).

In any case, from our side it's okay to document the limitation. – I've added a note to the description of the property.

my-tien requested a review fromemilykl

February 12, 2025 12:19

gvwilson assignedLiamConnors

Feb 18, 2025

Copy link

ContributorAuthor

my-tien commentedFeb 24, 2025

Hey guys, I assume the failing tests are unrelated to the changes in the PR, can you verify?

Copy link

Contributor

gvwilson commentedFeb 24, 2025

cc@emilykl to confirm that the failing tests aren't related to the changes in this PR - thanks

Copy link

stephprobst commentedApr 23, 2025

Hi@emilykl ,@gvwilson,

just following up on some old PRs. This seems to be good to go and only needs to be merged (after checking, that the failing tests are not related to the PR). Could you please have a look?

Thank you!