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

added fill to gpar in geom_curve() #2375

Merged
merged 5 commits into from Apr 27, 2018

Conversation

Projects
None yet
3 participants
@hrbrmstr
Contributor

hrbrmstr commented Dec 19, 2017

Based on https://stackoverflow.com/questions/47878787/fill-arrow-on-geom-curve-ggplot2/47879776#47879776

this adds a missing fill gpar (i.e. back on par with geom_segment) and takes care of ^^ issue.

I also added the fill aesthetic to geom_segment() and changed the fill there to use it.

this means the arrows can be filled in for both.

NOTE: @clauswilke had the idea to add the fill aesthetic to geom_curve and I figured geom_segment shldn't be left out.

hrbrmstr added some commits Dec 19, 2017

@@ -38,6 +38,7 @@ geom_curve <- function(mapping = NULL, data = NULL,
#' @usage NULL
#' @export
GeomCurve <- ggproto("GeomCurve", GeomSegment,
default_aes = aes(colour = "black", fill = "black", size = 0.5, linetype = 1, alpha = NA),

This comment has been minimized.

@hadley

hadley Dec 19, 2017

Member

I think you can omit this since it will be inherited from geom_segment()

@hadley

This comment has been minimized.

Member

hadley commented Dec 19, 2017

Can you please add a bullet to NEWS? It should briefly describe the change (starting with name of the function), and crediting yourself with (@yourname, #issuenumber).

I think this is a slight change to behaviour? i.e. if you had arrows and mapped the colour to a variable, the arrows will now be black instead of coloured?

@clauswilke

This comment has been minimized.

Member

clauswilke commented Dec 19, 2017

It is a change in behavior, and on further reflection making the arrow fill an aesthetic may cause problems for some.

First, under the old behavior geom_curve() would always fill the arrow head in white, regardless of any colour or fill settings. That's clearly wrong.

Second, the question is whether the arrow head should take the color of colour or the color of fill. If people currently draw filled arrows specified by colour and use fill for some other geom then they may get unexpected results. It might be better to use a non-standard aesthetic, such as arrow_fill, which by default mirrors colour. This then links back to the needs explained in issue #2345, but the change wouldn't cause any noticeable regressions.

Unless @hrbrmstr beats me to it, I can address this towards the end of the week. Not today, unfortunately.

@hadley

This comment has been minimized.

Member

hadley commented Dec 19, 2017

For now, I'd prefer to make this a parameter of the geom, and then we can figure out how to make it an aesthetic post-2345.

@clauswilke

This comment has been minimized.

Member

clauswilke commented Dec 19, 2017

So, it'd be a parameter that is set to NA or NULL by default, in which case the geom uses the colour aesthetic to fill the arrow, but if it's set to a color then that's used to override the fill color with that specific value? This sounds reasonable to me.

@hadley

This comment has been minimized.

Member

hadley commented Dec 19, 2017

Exactly - you can just copy what geom_boxplot() does with outlier.colour etc

@hrbrmstr

This comment has been minimized.

Contributor

hrbrmstr commented Dec 19, 2017

Aye. I'll make all those changes later today and also mod the docs + NEWS

@hrbrmstr

This comment has been minimized.

Contributor

hrbrmstr commented Dec 19, 2017

set.seed(123)
data <- data_frame(x = rnorm(10), y = rnorm(10))

gg <- ggplot(data, aes(x, y)) + geom_point()
gg + geom_curve(aes(x = 0, y = 0, xend = 1, yend = 1),
                color = "black",
                arrow = arrow(type = "closed"))

image

gg + geom_curve(aes(x = 0, y = 0, xend = 1, yend = 1),
                color = "black", arrow.fill = "red",
                arrow = arrow(type = "closed"))

image

gg + geom_segment(aes(x = 0, y = 0, xend = 1, yend = 1),
                  color = "black",
                  arrow = arrow(type = "closed"))

image

gg + geom_segment(aes(x = 0, y = 0, xend = 1, yend = 1),
                  color = "black", arrow.fill = "red",
                  arrow = arrow(type = "closed"))

image

and, for good measure

gg + geom_curve(aes(x = 0, y = 0, xend = 1, yend = 1),
                color = "#00000000", arrow.fill = "black",
                arrow = arrow(type = "closed"))

image

gg + geom_segment(aes(x = 0, y = 0, xend = 1, yend = 1),
                  color = "#00000000", arrow.fill = "red",
                  arrow = arrow(type = "closed"))

image

@hrbrmstr

This comment has been minimized.

Contributor

hrbrmstr commented Dec 19, 2017

Roxygen docs were also updated as well as the NEWS.md

NEWS.md Outdated
@@ -48,6 +48,10 @@
* Added `stat_qq_line()` to make it easy to add a simple line to a Q-Q plot. This
line makes it easier to judge the fit of the theoretical distribution
(@nicksolomon).
* Added `arrow.fill` parameter to `geom_segment()` and `geom_curve()` to

This comment has been minimized.

@hadley

hadley Dec 19, 2017

Member

Can you please rewrite so the function names come first? (And then use that to alphabetise within this list of changes?)

(Normally I do that just before release but this release has been dragging on because I haven't found the time to finish it off)

This comment has been minimized.

@hrbrmstr

hrbrmstr Dec 19, 2017

Contributor

Done.

hrbrmstr added some commits Dec 19, 2017

@hadley hadley merged commit 4aba305 into tidyverse:master Apr 27, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@hadley

This comment has been minimized.

Member

hadley commented Apr 27, 2018

Thanks!

@lock

This comment has been minimized.

lock bot commented Oct 24, 2018

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Oct 24, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.