Skip to content

Explicitly define all theme elements #3585

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Nov 14, 2019
Merged

Explicitly define all theme elements #3585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a851a56
explicitly define all theme elements. closes #3584.
clauswilke Oct 25, 2019
afafe98
generate and cache all null theme on load
clauswilke Oct 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 24 additions & 3 deletions R/theme-defaults.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ theme_grey <- function(base_size = 11, base_family = "",
# Throughout the theme, we use three font sizes, `base_size` (`rel(1)`)
# for normal, `rel(0.8)` for small, and `rel(1.2)` for large.

theme(
t <- theme(
# Elements in this first block aren't used directly, but are inherited
# by others
line = element_line(
Expand Down Expand Up @@ -234,6 +234,9 @@ theme_grey <- function(base_size = 11, base_family = "",

complete = TRUE
)

# make sure all elements are set to NULL if not explicitly defined
ggplot_global$theme_all_null %+replace% t
}
#' @export
#' @rdname ggtheme
Expand Down Expand Up @@ -455,7 +458,7 @@ theme_void <- function(base_size = 11, base_family = "",
half_line <- base_size / 2

# Only keep indispensable text: legend and plot titles
theme(
t <- theme(
line = element_blank(),
rect = element_blank(),
text = element_text(
Expand Down Expand Up @@ -508,6 +511,9 @@ theme_void <- function(base_size = 11, base_family = "",

complete = TRUE
)

# make sure all elements are set to NULL if not explicitly defined
ggplot_global$theme_all_null %+replace% t
}


Expand All @@ -518,7 +524,7 @@ theme_test <- function(base_size = 11, base_family = "",
base_rect_size = base_size / 22) {
half_line <- base_size / 2

theme(
t <- theme(
line = element_line(
colour = "black", size = base_line_size,
linetype = 1, lineend = "butt"
Expand Down Expand Up @@ -639,4 +645,19 @@ theme_test <- function(base_size = 11, base_family = "",

complete = TRUE
)

# make sure all elements are set to NULL if not explicitly defined
ggplot_global$theme_all_null %+replace% t
}

theme_all_null <- function() {
# set all elements in the element tree to NULL
elements <- sapply(
names(ggplot_global$element_tree),
function(x) NULL,
simplify = FALSE, USE.NAMES = TRUE
)

args <- c(elements, list(complete = TRUE))
do.call(theme, args)
}
Comment on lines +653 to 663
Copy link
Member

Choose a reason for hiding this comment

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

Could we cache this theme as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do that tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I'm also wondering whether this function should be exported, so external package developers can use the same trick, or whether we should rather encourage them to always start with one of the usable ggplot2 themes.

Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to guide them towards modifying an existing theme... They can always base it off theme_void() if they want a clean slate

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. In any case, I think we need a vignette or other document to provide best practices recommendations for themes. I'll probably write something like that when I'm done with the theme reworking.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did a quick check. It looks like all the most popular theme packages already always start from an existing theme. In fact, I couldn't find any example that didn't do this.

1 change: 1 addition & 0 deletions R/zzz.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ pathGrob <- NULL
.zeroGrob <<- grob(cl = "zeroGrob", name = "NULL")

# create default theme, store for later use, and set as current theme
ggplot_global$theme_all_null <- theme_all_null() # required by theme_grey()
ggplot_global$theme_grey <- theme_grey()
ggplot_global$theme_current <- ggplot_global$theme_grey

Expand Down
35 changes: 35 additions & 0 deletions tests/testthat/test-theme.r
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,3 +484,38 @@ test_that("plot titles and caption can be aligned to entire plot", {
expect_doppelganger("caption aligned to entire plot", plot)

})

test_that("provided themes explicitly define all elements", {
elements <- names(ggplot_global$element_tree)

t <- theme_all_null()
expect_true(all(names(t) %in% elements))
expect_true(all(vapply(t, is.null, logical(1))))

t <- theme_grey()
expect_true(all(names(t) %in% elements))

t <- theme_bw()
expect_true(all(names(t) %in% elements))

t <- theme_linedraw()
expect_true(all(names(t) %in% elements))

t <- theme_light()
expect_true(all(names(t) %in% elements))

t <- theme_dark()
expect_true(all(names(t) %in% elements))

t <- theme_minimal()
expect_true(all(names(t) %in% elements))

t <- theme_classic()
expect_true(all(names(t) %in% elements))

t <- theme_void()
expect_true(all(names(t) %in% elements))

t <- theme_test()
expect_true(all(names(t) %in% elements))
})