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

Update for ggplot2 3.5.0 #200

Merged
merged 5 commits into from
Jan 19, 2024
Merged

Update for ggplot2 3.5.0 #200

merged 5 commits into from
Jan 19, 2024

Conversation

teunbrand
Copy link
Contributor

Hi Claus,

As you might have noticed, we have been preparing a new release of ggplot2. During a reverse dependency check, it became apparent that the prospective ggplot2 3.5.0 would break cowplot.

This PR updates cowplot to work with the new ggplot2 version. It does the following things:

  • align_margins() is updated to find only simple 'null'-units. The guide-boxes might contain compound 'null'-units and shouldn't be considered the same as panels.
  • Updates alignment tests consider right/bottom alignment based on the number of columns/rows instead of absolute indices.
  • Updated a few other tests whose assumptions no longer hold.

What you still might consider but isn't currently breaking, is how get_legend() should handle multiple guide positions introduced by tidyverse/ggplot2#5488.

To test the code changes with the release candidate, you can install it with the code below:

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

The release of ggplot2 3.5.0 is scheduled for the 12th of Februari. The progress of the release can be tracked in tidyverse/ggplot2#5588. I hope that this PR might help cowplot get out a fix if necessary.

@clauswilke
Copy link
Contributor

Thanks! I'm impressed you went through all of this and fixed it. I was always worried this would break after your legend rewrite. I should have time next week to go over things in detail and prepare a release ahead of the ggplot2 release.

@teunbrand
Copy link
Contributor Author

Ah a few cracks but it's not fundamentally broken I'd say. Nothing that can't be glued back together :)

@@ -45,7 +45,8 @@ test_that("complex alignments, h, v, hv", {
expect_equal(plots[[1]]$widths[1:4], plots[[2]]$widths[1:4])

plots <- align_plots(p1, p2, align = "v", axis = "r", greedy = FALSE) # align right
expect_equal(plots[[1]]$widths[6:9], plots[[2]]$widths[14:17])
ncol <- vapply(plots, ncol, integer(1))
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's just a test but I don't really like having a variable (ncol) that looks the same as a built-in function (ncol). Could you change the variable name? n would be fine, as would be nc, or anything else really.

@clauswilke clauswilke merged commit a5bedc7 into wilkelab:master Jan 19, 2024
4 checks passed
@clauswilke
Copy link
Contributor

Thanks! I understand the issue with get_legend() but I don't really have the bandwidth to think about it right now so I'll leave it at the duct-taped solution. If it's a real issue people will start complaining.

@clauswilke
Copy link
Contributor

Just to confirm: This is on CRAN now. Will do ggridges next.

@teunbrand
Copy link
Contributor Author

Thank you Claus!

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