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

Code style in examples hard to read in reference docs #4092

Closed
rjake opened this issue Jun 25, 2020 · 4 comments · Fixed by #4098
Closed

Code style in examples hard to read in reference docs #4092

rjake opened this issue Jun 25, 2020 · 4 comments · Fixed by #4098

Comments

@rjake
Copy link
Contributor

rjake commented Jun 25, 2020


Really minor but the code could be easier to read in some of the examples.

In the sec_axis() example it looks ok if I copy and paste the code but in the help menu and the ref docs, it's hard to see where the parentheses start and end and the piping of the plot elements

When copied, it's easier to read:

ggplot(df, aes(x = dx, y = price)) + geom_line() +
  scale_x_datetime("Date", date_labels = "%b %d",
                   date_breaks = "6 hour",
                   sec.axis = dup_axis(name = "Time of Day",
                                       labels = scales::time_format("%I %p")))

I'd like for it to look like this:

ggplot(df, aes(x = dx, y = price)) +
  geom_line() +
  scale_x_datetime(
    name = "Date",
    date_labels = "%b %d",
    date_breaks = "6 hour",
    sec.axis = dup_axis(
      name = "Time of Day",
      labels = scales::time_format("%I %p")
    )
  )
@karawoo
Copy link
Member

karawoo commented Jun 25, 2020

I agree and it should be updated to follow tidyverse style. Would you be interested in opening a pull request? The relevant lines are here:

#' # This may useful for labelling different time scales in the same plot
#' ggplot(df, aes(x = dx, y = price)) + geom_line() +
#' scale_x_datetime("Date", date_labels = "%b %d",
#' date_breaks = "6 hour",
#' sec.axis = dup_axis(name = "Time of Day",
#' labels = scales::time_format("%I %p")))
#'
#' # or to transform axes for different timezones
#' ggplot(df, aes(x = dx, y = price)) + geom_line() +
#' scale_x_datetime("GMT", date_labels = "%b %d %I %p",
#' sec.axis = sec_axis(~ . + 8 * 3600, name = "GMT+8",
#' labels = scales::time_format("%b %d %I %p")))

@rjake
Copy link
Contributor Author

rjake commented Jun 26, 2020

Yes. I'll take it

@rjake
Copy link
Contributor Author

rjake commented Jun 26, 2020

@karawoo Should I go through all the examples? When done, do I rebuild the site? I've seen some repos ask contributors not to.

@yutannihilation
Copy link
Member

Should I go through all the examples?

You don't need to (though it would be really appreciated if you do so). ggplot2 has so massive amount of examples that we probably cannot check if your pull request is comprehensive or not. I personally think we should automatically style all the code by using styler at some point... Anyway you can pick only the examples that you want to fix.

When done, do I rebuild the site?

You don't need to rebuild the pkgdown site as it is automatically built by GitHub Actions CI after your pull request is merged. (If you meant "regenerate the docs," please do.)

Note that you need to install the specific branch of roxygen2 by devtools::install_github("r-lib/roxygen2#1109") to generate the docs for now. c.f. r-lib/roxygen2#1108

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

Successfully merging a pull request may close this issue.

3 participants