Skip to content
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

Smarter keys #5302

Closed
wants to merge 10 commits into from
Closed

Smarter keys #5302

wants to merge 10 commits into from

Conversation

teunbrand
Copy link
Collaborator

@teunbrand teunbrand commented May 9, 2023

This PR aims to fix #3648. This PR is mutually incompatible with #3649.

Briefly, it automatically checks whether the key value is actually in the data and doesn't draw keys when it isn't. This contrasts with the approach taken in #3649, where the user is given more control.

Less briefly: it performs this check when show.legend = NA, which is the default. When show.legend = TRUE, is draws the keys regardless of presence in the layer data. To get back the old behaviour and you have multiple legends, you'd have to be specific to include a layer in the keys of a particular legend, for example show.legend = c(colour = TRUE).

A small demo from #3648 (comment). Note how there is no point displayed for the 'regression line' key, and no line for the 'points' key.

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

ggplot(mtcars, aes(disp, mpg)) + 
  geom_point(aes(color = "points")) +
  geom_smooth(aes(color = "regression line"))
#> `geom_smooth()` using method = 'loess' and formula = 'y ~ x'

Created on 2023-05-09 with reprex v2.0.2

@@ -89,6 +89,7 @@ ggplot_build.ggplot <- function(plot) {
if (npscales$n() > 0) {
lapply(data, npscales$train_df)
data <- lapply(data, npscales$map_df)
plot$key_data <- npscales$key_data(data)
Copy link
Collaborator Author

@teunbrand teunbrand May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the data has to happen before the defaults are filled in and after_scale() modifications are applied. Otherwise the information might be lost. I gave the scales list a method to check the data. The downside is that we have to check all the data regardless of whether the scale has a legend guide, as that isn't resolved at this point.

}
present
}, logical(length(pal)))
list(aesthetics = aes, data = data_frame0(pal = pal, member = out))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point, member is a length(pal) * length(data) logical matrix tracking whether a palette value was seen.

Copy link
Collaborator Author

@teunbrand teunbrand May 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also aes can be a vector, e.g. c("colour", "fill"). The inclusion/exclusion rule in the guide only needs to match one of the aesthetics, not all, to be included.

@teunbrand teunbrand added feature a feature request or enhancement guides 📏 labels Jul 9, 2023
@teunbrand
Copy link
Collaborator Author

Closing this PR in favour of #5502

@teunbrand teunbrand closed this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement guides 📏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement smart merging of guides so inappropriate key glyphs are omitted
1 participant