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

Annotation logticks outside #3783

Merged
merged 13 commits into from Mar 5, 2020

Conversation

kbodwin
Copy link
Contributor

@kbodwin kbodwin commented Jan 31, 2020

Allows logticks to be outside of plotting pane, when used with coord_cartesian(clip = "off")

Fixes #3660

R/annotation-logticks.r Outdated Show resolved Hide resolved
R/annotation-logticks.r Outdated Show resolved Hide resolved
@hadley hadley added the tidy-dev-day 🤓 label Jan 31, 2020
@clauswilke
Copy link
Member

clauswilke commented Feb 1, 2020

Please merge in master so the CI doesn't break anymore. Thanks!

@thomasp85
Copy link
Member

thomasp85 commented Feb 3, 2020

Last step is to add a small note on the new feature to the NEWS.md file. After that it is good to go

NEWS.md Outdated
@@ -2,6 +2,9 @@

# ggplot2 3.3.0

* Added an `outside` option to `annotation_logticks()` that places tick marks
outside of the plot bounds. (#3783, @kbodwin)
Copy link
Member

@clauswilke clauswilke Feb 4, 2020

Choose a reason for hiding this comment

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

Thanks for making this change. Unfortunately I'll have to request one more modification. I think your news item is in the wrong location in the News file. This feature won't make it into ggplot2 3.3.0, as that version has already been branched off.

Could you move your entry to above the ggplot2 3.3.0 heading?

Copy link
Contributor Author

@kbodwin kbodwin Feb 4, 2020

Choose a reason for hiding this comment

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

oops, sorry!

@@ -7,6 +7,9 @@
#' @param sides a string that controls which sides of the plot the log ticks appear on.
#' It can be set to a string containing any of `"trbl"`, for top, right,
#' bottom, and left.
#' @param outside logical that controls whether to move the log ticks outside
#' of the plot area. Default is off (FALSE). You will also need to use
Copy link
Member

@clauswilke clauswilke Feb 4, 2020

Choose a reason for hiding this comment

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

Apologies, one last thing (I promise): Please put FALSE into backticks.

@thomasp85
Copy link
Member

thomasp85 commented Mar 3, 2020

@clauswilke and @kbodwin do you want to finish this off?

@clauswilke
Copy link
Member

clauswilke commented Mar 3, 2020

@kbodwin Please let me know if you're in a position to resolve the conflict and make the final edit. Otherwise I can probably go into your PR and make the final changes myself.

@clauswilke clauswilke self-assigned this Mar 3, 2020
@kbodwin
Copy link
Contributor Author

kbodwin commented Mar 5, 2020

Done; sorry to miss that last tweak.

@clauswilke clauswilke merged commit 02c2a9c into tidyverse:master Mar 5, 2020
1 of 2 checks passed
@clauswilke
Copy link
Member

clauswilke commented Mar 5, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tidy-dev-day 🤓
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants