-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
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.
Looks good! Thanks
I have two concerns about this:
|
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? |
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:
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. |
Yes, ignore my 2nd concern, it doesn't apply. However, for my 1st concern, a little more needs to be done. First, Line 116 in 7e6d125
and here: Line 121 in 7e6d125
Second, I doubt Finally, I wouldn't discount the fill variable, so the final sequence should read: |
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.
|
Looks great, thanks! Going forward, I suggest you check out the reprex package, so you don't have to copy and paste screenshots. |
Resolves: #3455
Below is reprex of this working:
New output:

Old output:
