Skip to content

Pass fill value to legend key icons #3781

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

Merged
merged 4 commits into from
Feb 2, 2020

Conversation

808sAndBR
Copy link
Contributor

Resolves: #3455

Below is reprex of this working:

# sample data
dat <- data.frame(
  x = as.factor(1:10),
  y = c(20,30,13,37,12,50,31,2,40,30),
  z = rep('a', 10)
)

# basic plot
ggplot(dat) +
  geom_segment(
    aes(x = x, xend = x, y = 0, yend = y+15, linetype = z), 
    arrow = arrow(length = unit(0.25, 'cm'), type = 'closed'),
    size = 0.7
  ) 

New output:
image

Old output:
image

Copy link
Member

@thomasp85 thomasp85 left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks

@clauswilke
Copy link
Member

I have two concerns about this:

  1. geom_segment() has a parameter arrow.fill. We should see if we can carry it through to the legend and use if defined.

  2. I'm wondering whether this draw key is used by any geoms that meaningfully use a fill aesthetic, and whether in this case filling the arrow in the way it is done now will create strange results. Possibly we need a new draw key specifically for geom_segment().

@thomasp85
Copy link
Member

I don't think we can do this for geom_segment specifically. Any segment and path type geom can have arrowheads and they should behave as expected.

Your concern about the arrow.fill argument is valid though... is there a straightforward way to pass this through?

@808sAndBR
Copy link
Contributor Author

I believe that this new version passes the arrow.fill through as expected, but let me know if it looks questionable.

For the second concern. I ran this version of legend_draw on everywhere that I saw importing it:

geom_sf
geom_errorbar
geom_path
geom_segment
geom_rug
geom_hline

None of the outputs seems strange to me, but I have them in a Rmd if you want to swing by and see if you notice anything strange.

@hadley hadley added the tidy-dev-day 🤓 Tidyverse Developer Day label Jan 31, 2020
@clauswilke
Copy link
Member

Yes, ignore my 2nd concern, it doesn't apply.

However, for my 1st concern, a little more needs to be done. First, arrow.fill should be the first choice for fill, not the second, as it is the case in the geom. See here:

arrow.fill <- arrow.fill %||% coord$colour

and here:
fill = alpha(arrow.fill, coord$alpha),

Second, I doubt data$arrow.fill will work. I think it has to be params$arrow.fill. In either case, I think it's important that you run a test to check whether this actually works.

Finally, I wouldn't discount the fill variable, so the final sequence should read:
params$arrow.fill %||% data$colour %||% data$fill %||% "black"

@808sAndBR
Copy link
Contributor Author

Thanks for all the extra info, it's been super helpful for understanding how this all works behind the scenes.

Ahhhh, I see I think it picking up "black" from the back of the options which made me think it was working as expected.

I have added some more robust test cases (below) to make sure this is working as expected. I think it looks right to me, but let me know if anything seems off.

# sample data
dat <- data.frame(
  x = as.factor(1:10),
  y = c(20,30,13,37,12,50,31,2,40,30),
  z = rep('a', 10)
)

# basic plot
ggplot(dat) +
  geom_segment(
    aes(x = x, xend = x, y = 0, yend = y+15, linetype = z), 
    arrow = arrow(length = unit(0.25, 'cm'), type = 'closed'),
    size = 0.7
  ) 

image

ggplot(dat) +
  geom_segment(
    aes(x = x, xend = x, y = 0, yend = y+15, linetype = z), 
    arrow = arrow(length = unit(0.25, 'cm'), type = 'closed'),
    arrow.fill = 'red',
    size = 0.7
  ) 

image

ggplot(dat) +
  geom_segment(
    aes(x = x, xend = x, y = 0, yend = y+15, linetype = z, colour='yellow'), 
    arrow = arrow(length = unit(0.25, 'cm'), type = 'closed'),
    size = 0.7
  ) 

image

ggplot(dat) +
  geom_segment(
    aes(x = x, xend = x, y = 0, yend = y+15, linetype = z, colour='yellow'), 
    arrow = arrow(length = unit(0.25, 'cm'), type = 'closed'),
    arrow.fill = 'blue',
    size = 0.7
  ) 

image

@clauswilke
Copy link
Member

Looks great, thanks!

Going forward, I suggest you check out the reprex package, so you don't have to copy and paste screenshots.

@clauswilke clauswilke merged commit 0cbb078 into tidyverse:master Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidy-dev-day 🤓 Tidyverse Developer Day
Projects
None yet
Development

Successfully merging this pull request may close these issues.

filled arrowhead not showing in legend
4 participants