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

Move CRS responsibility from GuideAxis to CoordSf #5440

Merged
merged 2 commits into from
Oct 2, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR doesn't fix any issue, but it amends #5293.

I have become mildly frustrated by the lines below in the axis guide:

ggplot2/R/guide-axis.R

Lines 138 to 141 in 69e7430

if (inherits(coord, "CoordSf")) {
# Positions already given in target crs
panel_params$default_crs <- panel_params$crs
}

Which I believe should be the coord's responsibility, not the axis' responsibility. This PR moves that ressponsibility from the axis to the coord.

R/coord-sf.R Outdated
x_range = x_range,
y_range = y_range,
graticule = graticule,
crs = params$crs,
default_crs = params$default_crs,
viewscales = viewscales
!!viewscales
Copy link
Collaborator Author

@teunbrand teunbrand Sep 27, 2023

Choose a reason for hiding this comment

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

This makes CoordSf$setup_panel_guides() redundant and we can rely on parental Coord$setup_panel_guides() instead.

@teunbrand teunbrand changed the title Move CRS responsibility to CoordSf Move CRS responsibility from GuideAxis to CoordSf Sep 27, 2023
Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

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

LGTM

@teunbrand teunbrand merged commit ad33741 into tidyverse:main Oct 2, 2023
12 checks passed
@teunbrand teunbrand deleted the coord_sf_polish branch October 2, 2023 08:50
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