theme: Add different ticks length for different axes

[pank]

Hi,

I would like to add support for different ticks length for different axes.
My main motivation is to match an Excel theme that I have been given, which features upwards ticks on the x-axis. I don't want inwards ticks on the y-axis though.

It was first requested in issue #1319.

I have not submitted patches to ggplot2 before, neither am I too familiar with the tidyverse style guide normally, nor the github pull interface (I normally use git format-patch. If I have broken any conventions let me know and I will try to fix the mistake.

It was not clear to me what to do where the names I added were longer than the length of other nearby names. The problem is that the alignment goes wrong.
Here is an example.

Should I,

• Fix the alignment issues in a subsequent commit? (still bad for git blame...)?
• Or find shorter names?
• Or just leave it be?

Also, I have not used visual tests before, but it seems to work. (I just ran devtools::test() and saw no more errors; the documentation is a bit sparse on visual tests).

Finally, this bit is pretty crude:

  theme$axis.ticks.length.x <- theme$axis.ticks.length.x %||% theme$axis.ticks.length
  theme$axis.ticks.length.x.bottom <- theme$axis.ticks.length.x.bottom %||% theme$axis.ticks.length.x
  theme$axis.ticks.length.x.top <- theme$axis.ticks.length.x.top %||% theme$axis.ticks.length.x
  theme$axis.ticks.length.y <- theme$axis.ticks.length.y %||% theme$axis.ticks.length
  theme$axis.ticks.length.y.left <- theme$axis.ticks.length.y.left %||% theme$axis.ticks.length.y
  theme$axis.ticks.length.y.right <- theme$axis.ticks.length.y.right %||% theme$axis.ticks.length.y

I just followed the example of legend.spacing.x, but if desired I can try to cut it down.

Below is a minimal example of a somewhat of useful thing that could be done after applying the patch. Notice the ticks on the button is sort-of enclosing the bar and the ones on the right axis serve as an explanation of the graph. (I'm not saying this is the best way to explain what is going on in this graph!)

Thanks,
Rasmus

library(ggplot2) # or load via devtools
theme_set(theme_light()+
          theme(axis.ticks.length.x = unit(-.25, "cm"),
                axis.ticks.length.y.right=unit(.5, "cm"),
                panel.border=element_blank(),
                panel.grid.major.x = element_blank(),
                panel.grid.minor.x = element_blank(),
                axis.text.x = element_text(hjust=-7,
                                           margin=margin(t=.5, , unit="cm"))))

set.seed(123)
d <- data.frame(t=rep(1:5, each=2),
                y = abs(rnorm(10)),
                type=rep(c("realized", "add-on"), 5))
p <- ggplot(d) +
    geom_rect(aes(xmin=-Inf, xmax=Inf, ymin=.25, ymax=2), fill="gray", alpha=.1) +
    geom_col(aes(t+.5, y, fill=type), position="stack", width=.5) +
    geom_hline(aes(yintercept=x), linetype="dashed",
               data=data.frame(x=c(.25,2))) + 
    coord_cartesian(expand=FALSE, ylim=c(0,2.5), xlim=c(1,5.75)) +
    guides(fill=FALSE) + xlab(NULL) + ylab(NULL) + 
    scale_y_continuous(sec.axis=sec_axis(~.*1,
                                         breaks=c(.25, 2, cumsum(tail(d$y, 2))),
                                         labels=c("minimum value",
                                                  "maximum value",
                                                  "current value",
                                                  "addon")))
p

[batpigandme]

Hi @pank,

Would you mind opening up a fresh issue for this? That way logistics/changes, etc. can be discussed without cluttering the PR itself.

Thanks!

PS As for styling, you can use the styler package to implement the in-house style as described in the tidyverse style guide.

[clauswilke]

I've wanted this option in the past, so I'd generally be in favor. Not sure if it can be done without the clutter of adding all these new theme elements.

For the theme calculation code, you could probably just write:

theme$axis.ticks.length.x.bottom <- theme$axis.ticks.length.x.bottom %||% theme$axis.ticks.length.x %||% theme$axis.ticks.length
theme$axis.ticks.length.x.top <- theme$axis.ticks.length.x.top %||% theme$axis.ticks.length.x %||% theme$axis.ticks.length

theme$axis.ticks.length.y.left <- theme$axis.ticks.length.y.left %||% theme$axis.ticks.length.y %||% theme$axis.ticks.length
theme$axis.ticks.length.y.right <- theme$axis.ticks.length.y.right %||% theme$axis.ticks.length.y  %||% theme$axis.ticks.length

Slightly less efficient, but also less cluttered and easier to read.

Don't worry about making multiple git commits into the same PR, they'll all be combined into one upon merge.

[thomasp85]

@clauswilke can you have a look at this one

[clauswilke]

Generally looks good to me. I left a few specific comments.

[clauswilke]

There's an extra "and" here.

[clauswilke]

Please use tidyverse indenting here (2 spaces only) and make sure there are two spaces around each =. Also, please add a comment explaining what this example shows (inward/outward pointing ticks on x/y axes).

[pank]

Dear Claus,

Thanks for the comments. I appreciate you taking the time to review this. Sorry about the silly mistakes. I guess I need to figure out a way to configure ESS to tidyverse style-guide if I find other opportunities to contribute to ggplot2.

I have pushed a new commit that resolves the language/styles issues. I added a description of the currently-present examples as well.

Should I rebase against upstream or does github take care of that aspect? Do I need to click any of the buttons above or will you re-review the patch?
[I'm more of a git am / git format-patch kind of guy and not experienced with Github].

Thomas, thanks for revising the PR.

A hope you'll both have a nice weekend.

Kind regards,
Rasmus

[clauswilke]

One small remaining nitpick: "inwards" instead of "upwards"? x-axis tick marks are always vertical, so arguably they always point up (and down).

[clauswilke]

I have one remaining minor wording issue, otherwise looks good to me.

[clauswilke]

You don't need to rebase, since github says there is no conflict with the base branch.

[pank]

Hi Claus,
I change the word per your suggestion.

Do I need to do anything else, e.g. a squash message, or is this sufficient?

Thanks,
Rasmus

[clauswilke]

Sorry to nitpick, one last request, then I'll merge: Please move the period to after (@pank, #2935), just like in the other news bullets.

[pank]

Ah, well spotted. Thanks a lot!

I noticed that the NEWS entry is currently filed under # ggplot2 3.1.0. But the PR has been tagged 3.2. Will the entry automatically be refilled to the right spot or should this be fixed? Sorry, I didn't find anything on NEWS entries in CONTRIBUTING or on the wiki...

Also, in the corresponding issue, you wrote

[Hadley,] [w]ould it make sense to add a function that can calculate [inheritance for] units [like axis.ticks.length] based on the theme and the inheritance tree?

Should we leave that for another patch?

[clauswilke]

I noticed that the NEWS entry is currently filed under # ggplot2 3.1.0. But the PR has been tagged 3.2. Will the entry automatically be refilled to the right spot or should this be fixed?

Thanks for noticing. This is actually a problem that needs to be fixed. It seems you're working from an old code base from before the last release. You'll have to rebase to or merge github master and then move your news item to above the 3.1.0 release.

Theme inheritance for units should be left to a separate patch, since it would involve a lot of other parts of the code.

[pank]

It seems you're working from an old code base from before the last release.

Indeed. It seems most of the patch is from October, 2018.

I just merge upstream ggplot2 as it was easier (no conflicts), and as I understand everything is anyway squashed once you merge the PR. If a rebase is preferred, I'll change it.

[clauswilke]

Thanks!

[lock]

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/