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 linejoin parameter to geom_segment. #2132

Merged
merged 4 commits into from
Jul 3, 2017
Merged

Conversation

Ax3man
Copy link
Contributor

@Ax3man Ax3man commented May 15, 2017

This PR simply adds support for the linejoin parameter of grid::segmentsGrob to geom_segment, just like the existing lineend parameter. This is especially useful when drawing large arrows that need to look sharp. The default of "round" can look poor with filled arrows as all corners are rounded off.

temp

The default is the current fixed value.

I'm not sure about the appropriate sizes etc. for the added example as it depends a bit on the size that is drawn for the online docs.

@Ax3man Ax3man changed the title Added linejoin parameter to geom_segment (#774). Added linejoin parameter to geom_segment. May 16, 2017
R/geom-segment.r Outdated
#' # Use lineend and linejoin to change the style of the segments
#' df2 <- expand.grid(lineend = c('round', 'butt', 'square'),
#' linejoin = c('round', 'mitre', 'bevel'), stringsAsFactors = FALSE)
#' segments <- lapply(seq_len(nrow(df2)), function(i) {
Copy link
Member

Choose a reason for hiding this comment

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

This use of lapply() will confuse most readers. Can you please generate with a simple helper function and copy and paste?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've simplified this is to a single call to geom_segment, hope that helps.

Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

That's much better thanks.

Can you please take a hard look to see if you can simplify the plot still further? I suggested a couple of places to start.

R/geom-segment.r Outdated
#' df2 <- expand.grid(lineend = c('round', 'butt', 'square'),
#' linejoin = c('round', 'mitre', 'bevel'), stringsAsFactors = FALSE)
#' df2 <- data.frame(df2, y = 1:9)
#' ggplot() +
Copy link
Member

Choose a reason for hiding this comment

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

Can you please use house indenting rules? (http://style.tidyverse.org/syntax.html#long-lines)

R/geom-segment.r Outdated
#' geom_segment(aes(x = 1, y = y, xend = 2, yend = y), df2, size = 4,
#' lineend = df2$lineend, linejoin = df2$linejoin,
#' arrow = arrow(length = unit(0.5, "inches"))) +
#' geom_text(aes(x = 2, y = 1:9, label = paste(df2$lineend, df2$linejoin)),
Copy link
Member

Choose a reason for hiding this comment

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

I think you can simplify this plot by setting data and aes(y = y) in the ggplot2 call. The geom_text() layer shouldn't use df$

@Ax3man
Copy link
Contributor Author

Ax3man commented Jun 1, 2017

I've simplified to a single aes call. I think it's useful to show all options available (as many users won't have much of an idea about what a bevel should look like), so that does make the plot a little more complicated than most. Alternatively I could just show e.g. two examples if you'd like to further simplify.

@hadley
Copy link
Member

hadley commented Jun 1, 2017

Looks good to me - thanks!

@hadley
Copy link
Member

hadley commented Jul 3, 2017

@karawoo can you please merge by hand?

@karawoo karawoo merged commit 1f25e57 into tidyverse:master Jul 3, 2017
@olli0601
Copy link

Hello, any chance the linejoin option could also be added to geom_curve? Thank you, Oliver

@lock
Copy link

lock bot commented Dec 23, 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 Dec 23, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants