Use the full range of the palette for binned viridis #4737
Conversation
R/scale-viridis.r
Outdated
| "viridis_b", | ||
| gradient_n_pal( | ||
| guide = "coloursteps", aesthetics = "colour", pal_full_range = FALSE) { | ||
| if(pal_full_range) { |
There was a problem hiding this comment.
Why does this need to be an option? Wouldn't you always want to use the full range?
There was a problem hiding this comment.
as per our discussion in #4372 with @clauswilke @yutannihilation: both ways of doing it could make sense
There was a problem hiding this comment.
Can you please provide an argument as to why the current behaviour would ever be desired?
There was a problem hiding this comment.
I can't say that I can. Maybe @clauswilke @yutannihilation?
There was a problem hiding this comment.
I have no idea. I was just saying the current behavior is understandable. I'm not sure what behavior is desirable, sorry.
There was a problem hiding this comment.
@thomasp85 should issue a final ruling, but I'm pretty sure the original behaviour was a mistake, and we should just always use the full range. We'll need to flag this as a visual change in the NEWS.
There was a problem hiding this comment.
Agree - we should just go with the approach in this PR. The original implementation was an error on my part
|
@gregleleu do you want to finish this off, given the feedback above? |
|
@thomasp85 Sure, so removing the |
|
@gregleleu sorry for missing the question — Yes exactly. This PR will implement the one true behaviour |
Removes argument to choose between behaviors
|
Thanks @gregleleu — as an advise for the future begin to develop PRs in their own separate branch instead of in main |
|
@thomasp85 |
|
The first basically. You keep the main branch as a complete copy of the upstream main and only use it as base for new branches to make PRs from. The usethis package has some helpful functions for this workflow. You basically use |
Resolves #4372
Adding a parameter to use the full width of the scale in scale_*_viridis_b functions.