Strip clipping optional (re: #4125) #4223
Conversation
|
Late answer to @clauswilke in the original PR... I personally feel that this belongs in themes, but not strongly so. Main motivation is that this seems more about making sure the look fits with the rest of the theme, rather than tweaking data display... But I can probably be persuaded by some good arguments 🙂 |
|
I'm fine with having this as an option in the theme. It just opens the question then of whether the clipping of the plot panel should also be an option in the theme. |
|
If it is defined in both places and the default is |
|
We should definitely only have it once. One of the cardinal sins of the current code base is that the same things can be defined and set so many places and we don't want to add to that |
|
I think we can argue that panel clipping is related to how the coordinate system display the data. panel clipping is often data dependent whereas strip clipping is theme dependent (in my mind) |
|
Let's move it into the theme. |
|
Would you like me to add panel clipping to the theme in this PR or is it better to postpone changes to panel clipping to a major release as it will likely break people's scripts? |
|
I meant "let's move strip clipping into the theme". I don't think we want to touch panel clipping at this time. @thomasp85 Does this align with your perspective? |
|
I apologise for my misunderstanding. The PR in it's current state has strip clipping in the theme, so I think it might be ready for review. Should I add a test that checks whether clipping can be turned off and on from the theme? |
|
Yep @clauswilke, we agree @teunbrand I'll give this PR a review once I'm back from vacation |
|
@teunbrand yes, please add a small test I forgot that this was slated for the feature release, so I'll hold off on reviewing it for now |
|
is there an ETA on this feature? I'd like to be able to use it |
If you're as impatient as I was, you could for now use this as a proxy. I'm hopeful that this PR will get some consideration before next release ^_^ |
|
Thank you! I was running into it with |
|
Any updates on this? |
tests/testthat/test-facet-strips.r
Outdated
| strip <- render_strips(labels, labeller = label_value, | ||
| theme = theme_test() + theme(strip.clip = "on")) | ||
| expect_equal(strip$x$top[[1]]$layout$clip, "on") | ||
|
|
||
| strip <- render_strips(labels, labeller = label_value, | ||
| theme = theme_test() + theme(strip.clip = "off")) | ||
| expect_equal(strip$x$top[[1]]$layout$clip, "off") |
There was a problem hiding this comment.
Can you please style like https://style.tidyverse.org/syntax.html#long-lines ?
R/theme.r
Outdated
| @@ -148,6 +148,9 @@ | |||
| #' @param strip.placement placement of strip with respect to axes, | |||
| #' either "inside" or "outside". Only important when axes and strips are | |||
| #' on the same side of the plot. | |||
| #' @param strip.clip should strip background edges and strip labels be clipped | |||
| #' to the extend of the strip background? Options are `"on"`, `"off"` and | |||
| #' `"inherit"`. | |||
There was a problem hiding this comment.
It means the same thing as for the clip argument in grid::viewport(): it inherits the clipping from the parent viewport. It was suggested to allow this in this comment.
There was a problem hiding this comment.
Can you explain that in the docs?
|
Thanks Hadley! I'm a bit busy coming days, but I'll make the changes once I have some extra time. |
|
Alright, I think this is about ready :) |
This should be PR #4125 addressing issue #4118, but I'm clumsy with Github.
I wanted to switch the commits for that PR to a different branch on my fork, but I didn't realise that this would automatically close the old PR. This is the same PR from a different branch.
I'm sorry for the inconvenience.