Repository navigation
Theme palettes warn about options - #6334
Merged
Merged
Conversation
teunbrand
commented
Feb 17, 2025
| validate = TRUE) { | ||
| elements <- find_args(..., complete = NULL, validate = NULL) | ||
|
|
||
| elements <- fix_theme_deprecations(elements) |
Collaborator
Author
There was a problem hiding this comment.
I've separated this in its own function as all these deprecations became somewhat distracting in ascertaining what theme() actually does.
Merge branch 'main' into theme_check_palettes # Conflicts: # R/theme.R # tests/testthat/test-theme.R
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR aims to fix #6333.
Briefly,
theme()checks whether colour palettes have been set simultaneously in the options and emit a warning when they are.RE: my earlier comment:
I think
theme()is the more appropriate place because:theme()has access to the theme palette and the optionsscale_colour_*()never gets access to the theme, and the alternative would beScalesList$set_palettes(), at which point it would be too late the change the entire scale rather than just the palette.options()is more used in a global fashion thantheme(). The case that we'd be missing now is if we do atheme_set()with a palette beforeoptions().check_theme(), which runs every render.Using theme without options is silent:
Using options without theme settings is silent:
But using both emits a warning.
Created on 2025-02-17 with reprex v2.1.1