-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Tags at panels #5167
Tags at panels #5167
Conversation
R/plot-build.r
Outdated
# Initialise the tag margins | ||
table <- gtable_add_padding(table, unit(0, "pt")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patchwork for some reason assumes this padding is present regardless of whether it was needed, so I left this in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, thanks... We should generally try to be predictable wrt the gtable structure that ggplot2 produces as it makes aligning multiple plots easier for patchwork
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes good sense to me :)
R/plot-build.r
Outdated
if (top) table$heights <- unit.c(height, table$heights[-1]) | ||
if (left) table$widths <- unit.c(width, table$widths[-1]) | ||
if (right) table$widths <- unit.c(table$widths[-n_col], width) | ||
if (bottom) table$heights <- unit.c(table$heights[-n_row], height) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pattern was some workaround for grid units not handling subassignment in R3.2 and earlier. Can we subassign units now?
With this PR there will be 3 different placement strategies for tags. Either in the margin (adding space for it), on top of the full plot area (not adding space), or in the panel area (not adding space). Do all three have a name in this new API? |
Currently, not all three placements have names, but for consistency's sake, maybe they should. We could make it so that we have the following names:
|
I also found that if you use the margin placement, To illustrate: library(ggplot2)
p <- ggplot(mpg, aes(displ, hwy)) +
geom_point() +
labs(tag = "Foobar") In the plot below, I'd expect the tag to be in the top left, with a left margin for the plot, but not a top margin for the plot.
Likewise, here I would have expected the text to be at the top right, with a plot margin on top but not on the right. p + theme(plot.tag.position = "top",
plot.tag = element_text(hjust = 1)) Created on 2023-03-21 with reprex v2.0.2 I feel that this might be a separate issue, but since I'm tinkering with this code anyway, might as well include adherence to |
I like the "margin" name and think we should include that for completeness... As for alignment, it works "correctly" right now in that it is perfectly aligned to the textbook that closely wraps the tag text. It is not given that the alignment is relative to the panel size. I can see the argument for having it as you describe because the alignment is right now useless but I don't think it is obviously a bug as it is |
One could now do these things: devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
p <- ggplot(mpg, aes(displ, hwy)) +
geom_point() +
labs(tag = "Foobar")
p + theme(plot.tag.position = c("panel", "topleft")) p + theme(plot.tag.position = c("plot", "bottomright")) p + theme(plot.tag.position = c("margin", "top")) Created on 2023-03-27 with reprex v2.0.2 |
Can we somehow make this new alignment work with numeric positions..? |
We could in theory, but the way it is setup right now it isn't straightforward, as we cannot pass a mixed type character/numeric to the devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
p <- ggplot(mpg, aes(displ, hwy)) +
geom_point() +
labs(tag = "Foobar")
p + theme(plot.tag.position = c("panel", "top"),
plot.tag = element_text(hjust = 0.75)) Created on 2023-04-15 with reprex v2.0.2 I think we might be able to extend this by not having the position default to 'topleft' for the 'panel' setting. |
yeah, I know the limitations of the current setup. I was thinking we should maybe considering a different setup such as providing an additional theme element (e.g. |
We could make that work, but how should a numeric value interact with |
yeah, that's a good question... But I think having that not work (and documented) is better than the alternative |
I'm just throwing an error now with numeric |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR aims to fix #4297.
Briefly, it adds the option to use
plot.tag.position = c({position}, "panel")
and draw the plot tag in the panel. It should also work with the patchwork package.Created on 2023-01-30 with reprex v2.0.2
Some side-notes:
ggplot_gtable.ggplot_built()
method. I refactored a little bit along the way.