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

GR: draw gridlines below all axes #4945

Merged
merged 1 commit into from
Jul 13, 2024

Conversation

dd0
Copy link
Contributor

@dd0 dd0 commented Jun 18, 2024

Description

GR currently plots all components of the x-axis first, and then the y-axis (and possibly z). Since horizontal gridlines belong to the y-axis, if there's a tick at the low end of xrange, the corresponding gridline will cover the x-axis (see issue #4202).

This fixes the issue for GR by splitting gr_draw_axis into three stages, so that we first draw all minor gridlines, then all major gridlines, and the rest of the axis on top.

With the change, a few image tests would need to be updated, in which the x-axis colour is now no longer affected by the (transparent) overlapping gridline -- as far as I can see, no visible changes. I assume that this is done separately in https://github.com/JuliaPlots/PlotReferenceImages.jl/?

Attribution

Things to consider

  • Does it work on log scales?
  • Does it work in layouts?
  • Does it work in recipes?
  • Does it work with multiple series in one call?
  • PR includes or updates tests?
  • PR includes or updates documentation?

Separates grid drawing from the remainder of gr_draw_axis, so that all
gridlines are drawn below the axis. Previously, y-axis gridlines were
drawn after the x-axis and could overlap it.

Fixes issue JuliaPlots#4202 for GR.
Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.70%. Comparing base (fa65e7d) to head (849dc95).
Report is 17 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4945      +/-   ##
==========================================
- Coverage   89.77%   89.70%   -0.07%     
==========================================
  Files          40       40              
  Lines        8780     8814      +34     
==========================================
+ Hits         7882     7907      +25     
- Misses        898      907       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BeastyBlacksmith
Copy link
Member

I assume that this is done separately in JuliaPlots/PlotReferenceImages.jl?

Yes, that is correct

@BeastyBlacksmith
Copy link
Member

I set that up for you in JuliaPlots/PlotReferenceImages.jl#151
Can you open a second PR to the v2 branch with the same changes?

@dd0 dd0 mentioned this pull request Jul 12, 2024
7 tasks
@dd0
Copy link
Contributor Author

dd0 commented Jul 12, 2024

Sorry about taking so long to respond -- I now made a second PR with essentially the same changes for v2.

@BeastyBlacksmith BeastyBlacksmith merged commit 8b9bcf2 into JuliaPlots:master Jul 13, 2024
12 of 17 checks passed
@BeastyBlacksmith
Copy link
Member

Thanks! Consider adding your name to our .zenodo.json file (see #3503 for reference)

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

Successfully merging this pull request may close these issues.

2 participants