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

Background for theme_void() and theme_minimal() #6345

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #6251.

Briefly, some themes had plot.background = element_blank() (explicitly or implicitly), which led to counterintuitive colouring.
By providing this element we surprise less.

Examples:

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

p <- ggplot(mpg, aes(displ, hwy)) +
  geom_point()

p + theme_minimal(paper = "cornsilk")

p + theme_void(paper = "cornsilk")

Created on 2025-02-24 with reprex v2.1.1

Comment on lines +562 to +565
rect = element_rect(
fill = paper, colour = NA, linewidth = 0, linetype = 1,
inherit.blank = FALSE
),
Copy link
Member

Choose a reason for hiding this comment

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

Curious why you are not just setting plot.background to a non-blank element?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right to wonder about this! Some test was protesting that we couldn't inherit from a blank root element. I then tried plot.background = element_rect(..., inherit.blank = FALSE) , but that gets overruled by theme(..., complete = TRUE). I then tried using +theme(...) at the end of the function to force inherit.blank = FALSE without being bound to complete = TRUE, but this felt like a dirty solution. I've landed on the current solution because at least it is straightforward.

Copy link
Member

Choose a reason for hiding this comment

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

that makes sense (in its own little way)

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

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.

Should theme_void() include paper argument? ink argument not working with geoms
2 participants