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

Key background inherits panel background #5551

Merged
merged 5 commits into from
Nov 29, 2023

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #5549.

Briefly, it sets legend.key to inherit from panel.background and eliminates the legend.key element from the built-in themes.
This causes the key backgrounds to automatically adhere to the panel background style.

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

ggplot(mpg, aes(displ, hwy)) +
  geom_point(aes(shape = factor(cyl))) +
  theme(
    panel.background = element_rect(fill = "cornsilk")
  )

Created on 2023-11-29 with reprex v2.0.2

For many built-in themes, this was already achieved by encoding the legend.key element and panel.background seperately in the same style. With this PR, this doesn't need to happen seperately anymore.

However, because the default theme_gray() had a slightly lighter legend.key than panel.background, there is a noticible (but not very obvious) visual change in this theme. See attached example where the left part is the current default and the right part is the default with this PR:

image

@teunbrand teunbrand added the visual change 👩‍🎨 Rendering change that will affect look of output label Nov 29, 2023
Copy link
Member

@clauswilke clauswilke left a comment

Choose a reason for hiding this comment

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

Surprisingly large patch for such a small change. Great idea and looks good to me.

Now looking at the old and new outputs side by side, I'd say the old one (with slightly lighter background in the legend) was wrong. You're coloring a smaller surface area, so if at all it should be darker not lighter. But matching is perfectly fine.

@teunbrand
Copy link
Collaborator Author

teunbrand commented Nov 29, 2023

Thanks for the review and kind words Claus! I agree with your assessment about surface area vs intensity, which would be easier to implement if we could do something like element_rect(fill = ~colorspace::darken(.x, 0.2)) to adjust settings on the fly.

@teunbrand teunbrand merged commit a96e664 into tidyverse:main Nov 29, 2023
@teunbrand teunbrand deleted the key_inherits_panel_background branch November 29, 2023 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
visual change 👩‍🎨 Rendering change that will affect look of output
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Should legend.key inherit from panel.background?
2 participants