-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Explicitly define all theme elements #3585
Conversation
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) | ||
} |
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.
Could we cache this theme as well?
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.
Sure, will do that tomorrow.
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.
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.
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 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
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.
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.
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 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.
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 old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
1 similar comment
This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/ |
This PR closes #3584 by making sure every theme explicitly defines every possible theme element. The strategy is to simply set every element that hasn't been defined explicitly to
NULL
. This can be done programmatically in a few lines of code.