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.

Sign up for GitHub

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

deal with old and new guides #54

Closed
wants to merge 1 commit into from
Closed

deal with old and new guides #54

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions R/sideCoord--utils.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,14 @@

panel_guides_grob <- function (guides, position, theme)
{
guide <- guide_for_position(guides, position) %||% guide_none()
guide_gengrob(guide, theme)
if (!inherits(guides, "Guides")) {
guide <- guide_for_position(guides, position) %||% guide_none()
guide_gengrob(guide, theme)
} else {
pair <- guides$get_position(position)
pair$guide$draw(theme, params = pair$params)
}

}

as_ggside_axis <- function(x) {
Expand All @@ -23,9 +29,20 @@ guide_for_position <- function (guides, position)
}

ggside_panel_guides_grob <- function(guides, position, theme) {
guide <- guide_for_position(guides, position) %||% guide_none()
guide <- as_ggside_axis(guide)
guide_gengrob(guide, theme)
if (inherits(guides, "Guides")) {
pair <- guides$get_position(position)
# # To use new guide system activate the line below
# pair$guide$draw(theme, params = pair$params)
# # And deactivate the line below up until the `} else {` line
if (inherits(pair$params$angle, "waiver")) {
pair$params$angle <- NULL
}
guide_gengrob.ggside_axis(pair$params, theme)
Comment on lines +34 to +40
Copy link
Author

Choose a reason for hiding this comment

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

Using the new system would result in some visual changes related to the spacing around axes.
I didn't fully understand where this change would come from, so I retained the old drawing method.
If you want to switch to the new guide system, you can use pair$guides$draw() instead.

Copy link
Owner

Choose a reason for hiding this comment

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

I appreciate that you have taken the time to look into correcting these. I have been working on bringing ggside up to speed with ggplot2 3.5.0 on the dev branch. I do not know which branch you were testing against, but my guess would be the main branch.

The dev branch should be compatible with ggplot2, but it isn't in a state where I'm ready to release. I've been putting a majority of my refactoring into dev-refactor which will eventually be merged to dev and then main.

I think in the future I will follow ggplot2's coding strategy and leave main as the development branch that is out of sync with CRAN.

I have switched to the new guide system where relevant in ggside and just ran ggside's tests against

remotes::install_github("tidyverse/ggplot2", ref = remotes::github_pull("5592"))

Only about 12 tests out of 275 had failures in that there was more spacing given to the Y axis title when the axis was plotted on the right. I for one can live with these changes 👍

Thanks again for your diligent work!

} else {
guide <- guide_for_position(guides, position) %||% guide_none()
guide <- as_ggside_axis(guide)
guide_gengrob(guide, theme)
}
}


Expand Down