-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Flexible palettes #6216
Flexible palettes #6216
Conversation
Includes fix for #6289, since it'd touch the same code. |
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 know the proposed changes don't practically affect the evaluation but I prefer to collect logical tests that define a specific condition (if that makes sense)
Co-authored-by: Thomas Lin Pedersen <thomasp85@gmail.com>
R/scale-colour.R
Outdated
|
||
has_old_args <- any(names(enexprs(...)) %in% c("h", "c", "l", "h.start", "direction")) | ||
|
||
if (has_old_args || !is.null(type) && is.null(palette)) { |
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 know I'm being pedantic here but please make the same changes to the logic tests here and below :-)
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.
Ah my bad, I should've spotted these!
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 PR aims to fix #6064 and fix #4910 and fix #6289 and fix #5843 and replaces #6112.
Briefly, it attempts to cast non-null
palette
arguments withas_discrete_pal()
/as_continuous_pal()
. These are currently shims.This PR replaces the older one because I could implemented more cleanly now that #5946 is merged.