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

Added new function geom_curve #1088

Merged
merged 3 commits into from Jun 12, 2015
Merged

Added new function geom_curve #1088

merged 3 commits into from Jun 12, 2015

Conversation

@veraanadi
Copy link
Contributor

@veraanadi veraanadi commented Apr 25, 2015

No description provided.

#' \Sexpr[results=rd,stage=build]{ggplot2:::rd_aesthetics("geom", "curve")}
#'
#' @inheritParams geom_point
#' @param curvature see curveGrob

This comment has been minimized.

@hadley

hadley Jun 11, 2015
Member

I think @inheritParams grid::curveGrob would be more useful here

#' b + geom_curve(aes(x = 5, y = 30, xend = 3.5, yend = 25), arrow = arrow(length = unit(0.5, "cm")))


geom_curve <- function (mapping = NULL, data = NULL, stat = "identity",

This comment has been minimized.

@hadley

hadley Jun 11, 2015
Member

Can you please make curvature, angle, etc explicit arguments? I think it leads to better documentation.

arrow = arrow)
))
}
print("geom_curve is not implemented for non-linear coordinates")

This comment has been minimized.

@hadley

hadley Jun 11, 2015
Member

Should probably be warning(), not print()


data <- remove_missing(data, na.rm = na.rm,
c("x", "y", "xend", "yend", "linetype", "size", "shape", "curvature", "angle", "ncp"),
name = "geom_curve")

This comment has been minimized.

@hadley

hadley Jun 11, 2015
Member

Ooooh, I see that you're passing them in an aesthetics. I think parameters would make more sense to me - I don't think mapping (e.g.) curvature to a variable is likely to yield a useful plot. (Plus if they're aesthetics you really need to think about default scales and legends etc)

@hadley
Copy link
Member

@hadley hadley commented Jun 11, 2015

Overall idea looks good.

Also needs a merge/rebase and a bullet point in NEWS

@veraanadi veraanadi force-pushed the veraanadi:master branch from 2c9afd3 to 99bc4c5 Jun 12, 2015
@veraanadi
Copy link
Contributor Author

@veraanadi veraanadi commented Jun 12, 2015

I hope I understood your suggestions correctly. I tried to include everything and did a merge/rebase and updated NEWS. If something still needs to be done, just let me know.

@@ -50,6 +53,8 @@ ggplot2 1.0.1.9000
* Improved documentation for `aes()` and many geoms and scales. I've tried
to reduce the use of `...` so that you can see all the documentation in one
place rather than having to navigate through multiple pages.
* Added new function geom_curve to add curved lines to plot (similar to

This comment has been minimized.

@hadley

hadley Jun 12, 2015
Member

Looks like something went wrong here - the bullet is duplicated

#' @section Aesthetics:
#' \Sexpr[results=rd,stage=build]{ggplot2:::rd_aesthetics("geom", "curve")}
#'
#' @inheritParams grid::curveGrob

This comment has been minimized.

@hadley

hadley Jun 12, 2015
Member

You need to keep the previous @inheritParams and this @inheritParams grid::curveGrob. Then you don't need to document the individual arguments below because they'll be automatically pulled from curveGrob's documentation

curvature=curvature, angle=angle, ncp=ncp,
square = FALSE, squareShape = 1,
inflect = FALSE, open = TRUE,
gp = gpar(col=alpha(colour, alpha), fill = alpha(colour, alpha),

This comment has been minimized.

@hadley

hadley Jun 12, 2015
Member

Should you be setting line fill colour?

@veraanadi veraanadi force-pushed the veraanadi:master branch from 41da17e to 039cd01 Jun 12, 2015
@veraanadi
Copy link
Contributor Author

@veraanadi veraanadi commented Jun 12, 2015

Thanks for your patience. I hope, this time everything is OK.

@hadley
Copy link
Member

@hadley hadley commented Jun 12, 2015

Looks good - thanks!

hadley added a commit that referenced this pull request Jun 12, 2015
Added new function geom_curve
@hadley hadley merged commit c582922 into tidyverse:master Jun 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lock
Copy link

@lock lock bot commented Jan 19, 2019

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 Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants