Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
Already on GitHub? Sign in to your account
show.legend now handles named logical vectors #2027
Conversation
|
This is not working as intended, I'll try and fix it. |
| @@ -247,9 +254,16 @@ guide_geom.legend <- function(guide, layers, default_mapping) { | ||
| guide$geoms <- plyr::llply(layers, function(layer) { | ||
| matched <- matched_aes(layer, guide, default_mapping) | ||
| + layer$show.legend <- unlist(rename_aes(as.list(layer$show.legend))) |
| if (length(matched) > 0) { | ||
| # This layer contributes to the legend | ||
| - if (is.na(layer$show.legend) || layer$show.legend) { | ||
| + include <- ifelse( |
hadley
Jul 3, 2017
Owner
I think this should be an if statement with separate then and else blocks. Can you please rewrite?
| @@ -16,6 +16,25 @@ test_that("Colorbar respects show.legend in layer", { | ||
| p <- ggplot(df, aes(x = x, y = y, color = x)) + | ||
| geom_point(size = 20, shape = 21, show.legend = TRUE) | ||
| expect_true("guide-box" %in% ggplotGrob(p)$layout$name) | ||
| + | ||
| + | ||
| + df <- data.frame(x = 1:3, y = 20:22) |
| + geom_point(size = 20) | ||
| + expect_length({ | ||
| + g <- ggplotGrob(p) | ||
| + g$grobs[[which(g$layout$name == "guide-box")]] |
hadley
Jul 3, 2017
Owner
I think you should pull this out into a helper function. Something like:
n_legends <- function(g) {
sum(g$layout$name == "guide-box")
}Then you can use expect_equal() consistently.
|
@Tutuchan are you interested in finishing off this PR? |
|
Yes, sorry, I have been busy these past couple of weeks. I'll get to it today. |
hadley
reviewed
Jul 14, 2017
Looking good. Just a few more minor changes.
Can you please also add a bullet describing the change to NEWS.md? And update the documentation for the show.legend parameter?
| - if (is.na(layer$show.legend) || layer$show.legend) { | ||
| + | ||
| + # rename color to colour | ||
| + layer$show.legend <- unlist(rename_aes(as.list(layer$show.legend))) |
hadley
Jul 14, 2017
Owner
I think this rename would be better off inside the branch where names are found.
Also why are you converting to a list and back?
Tutuchan
Jul 14, 2017
Contributor
When I first tested this, I thought the rename_aes() call was not working as intended because layer$show.legend is a named vector and not a list. But it does actually work so I must have been mistaken. I'm fixing this.
| + | ||
| + # check if this layer should be included, different behaviour depending on | ||
| + # if show.legend is a logical or a named logical vector | ||
| + include <- if (!is.null(names(layer$show.legend))) { |
| + g <- ggplotGrob(p) | ||
| + length(g$grobs[[which(g$layout$name == "guide-box")]]) - 1 | ||
| + } | ||
| + has_legends <- function(p){ |
Tutuchan
Jul 14, 2017
Contributor
You mean removing the has_legends() test but keeping the n_legends() function the same ?
| @@ -25,6 +25,9 @@ | ||
| * `geom_smooth`'s message for `method="auto"` now reports the formula used, | ||
| in addition to the name of the smoothing function (@davharris #1951). | ||
| + | ||
| +* The `show.legend` parameter now accepts a named logical vector to hide/show only some aesthetics in the | ||
| +legend (@tutuchan) |
| - if (is.na(layer$show.legend) || layer$show.legend) { | ||
| + | ||
| + # rename color to colour | ||
| + layer$show.legend <- rename_aes(layer$show.legend) |
hadley
approved these changes
Jul 14, 2017
Perfect, thanks!
@karawoo would you mind handling the merge conflict in the NEWS?
karawoo
merged commit 4b235fb
into
tidyverse:master
Jul 14, 2017
|
Great, thanks for the guidance @hadley ! |
Tutuchan commentedFeb 2, 2017
This is a followup to #1798.
The
rename_aescall is used to handle color/colour.