fix wrong guides for null unit key size

[Yunuuuu]

fix #389

[Yunuuuu]

p1 <- ggplot(mtcars) +
  geom_point(aes(mpg, disp)) +
  ggtitle("Plot 1")

p3 <- ggplot(mtcars) +
  geom_point(aes(hp, wt, colour = mpg)) +
  ggtitle("Plot 3")
wrap_plots(
  p1 + p3 + scale_color_continuous(guide = guide_colorbar(
    theme = theme(legend.key.height = unit(1, "null"))
  )),
  guides = "collect"
)

[thomasp85]

The build failures are being fixed on main so no need to worry about those

[Yunuuuu]

Thanks for your review

[thomasp85]

I would love to get this into the next release. I'm planning on starting submission on friday so if you have the possibility of addressing the last question beforehand I would be grateful. If not, no worries and we will just merge it after release

[Yunuuuu]

Hi, @thomasp85, it’s not necessary to use calc_element here, because we always re-define the values of legend.spacing in line 136-138 (function assemble_guides), this is what package_box in ggplot2 does.

[Yunuuuu]

I'm sorry to later reply, I replied on the the review comment, it seems you cannot see the message (marked as the Pending label).

[thomasp85]

Ah - I see. Then all is good

[thomasp85]

Can I get you to add a bullet to NEWS.md?

[teunbrand]

Can I just quickly interject that calc_element() was added in #365 for compatibility with the development version of ggplot2. I recommend checking wether {patchwork} tests succeed with current dev ggplot2.

[thomasp85]

thanks - yes, I remembered something like this. Can I get you to change it to using calc_element() @Yunuuuu ?

[Yunuuuu]

Thanks for the recomment, I'll make the updates.

[thomasp85]

Another time (just to save you some pain) make sure to create a new branch in your fork for a PR, rather than working directly from main. the usethis package has a bunch of pr_*() functions that steers you into a happy workflow :-)

[thomasp85]

Thank you for your help in this as well as pushing me to do more about free()

[Yunuuuu]

Thanks for the review and all of the advices!