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

Themes accept header font family #5887

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

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #5886.

Briefly, all themes gain a header_family argument that defaults to base_family. In addition, plot.subtitle and plot.caption no longer inherit from title but the root text element to follow #5886 (comment).

A demonstration:

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

ggplot(mpg, aes(displ, hwy)) +
  geom_point(aes(colour = class)) +
  labs(
    title = "Fuel efficiency",
    subtitle = "Described for 234 cars from 1999 and 2008.",
    caption = "Source: U.S. Environmental Protection Agency",
    tag = "A"
  ) +
  theme_gray(header_family = "Impact")

Created on 2024-05-10 with reprex v2.1.0

@thomasp85
Copy link
Member

I kind of understand why you want to change the inheritance of subtitle and caption but I'm afraid it would throw users off (especially for subtitle). I now regret we didn't name it "description" or something like that

@teunbrand
Copy link
Collaborator Author

So should we revert the change in inheritance and manually set the header font to the relevant elements in theme_*() functions?

@thomasp85
Copy link
Member

I'm not sure, but I think it would be best at least for subtitle. I'm fine with caption not inheriting from title, I think that was a wrong choice from the start

@tidyverse tidyverse deleted a comment from teunbrand May 21, 2024
@teunbrand
Copy link
Collaborator Author

teunbrand commented May 21, 2024

After some thought, I think it would be best to have the header_family = NULL be the default.
That way, you don't bake in header_family = "", which is convenient when you're looking to globally change the text font.

Example with the current PR, notice the titles remaining sans serif:

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

p <- ggplot(mpg, aes(displ, hwy)) +
  geom_point(aes(colour = class)) +
  labs(
    title = "Fuel efficiency",
    subtitle = "Described for 234 cars from 1999 and 2008.",
    caption = "Source: U.S. Environmental Protection Agency",
    tag = "A"
  ) +
  theme_gray()

p + theme_gray() + 
  theme(text = element_text(family = "Times New Roman"))

However, this all resolves nicely when the header font is NULL, notice the all serif text:

p + theme_gray(header_family = NULL) +
  theme(text = element_text(family = "Times New Roman"))

Created on 2024-05-21 with reprex v2.1.0

teunbrand and others added 4 commits May 21, 2024 14:37
Merge branch 'main' into header_family

# Conflicts:
#	R/theme-defaults.R
#	tests/testthat/test-theme.R
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.

Have header font be part of theme specification
2 participants