Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Add inherit.blank argument to element constructors #1754

Merged
merged 10 commits into from Sep 21, 2016

Conversation

Projects
None yet
3 participants
Collaborator

thomasp85 commented Sep 16, 2016

This PR is an alternative fix to #1555, #1557, #1565, and #1567 compared to #1581.

It will propose the inclusion of an inherit.blank argument to the element_* constructors. The reason for this is that we might want to suppress drawing of a parent element and all its children, and then reenable some of the children again. This requires element calculation to sometimes ignore element_blank() and sometimes not. The default should be inherit.blank = FALSE while elements in the included themes should have inherit.blank = TRUE instead.

@thomasp85 thomasp85 Add inherit.blank argument to element constructors
fcf9081

hadley added the in progress label Sep 16, 2016

Owner

hadley commented Sep 16, 2016

This seems like a reasonable approach to me

@thomasp85 thomasp85 News bullet
aaa64e5
Collaborator

thomasp85 commented Sep 17, 2016

@hadley can you review?

Now you can do stuff like this (not a beautiful plot but an example):

p + theme_minimal() + theme(line = element_blank(), axis.line.y = element_line())

rplot

Owner

hadley commented Sep 17, 2016

What if we automatically set inherit.blank if complete = TRUE in theme()?

R/theme-elements.r
@@ -13,6 +13,8 @@
#' @param fill Fill colour.
#' @param colour,color Line/border colour. Color is an alias for colour.
#' @param size Line/border size in mm; text size in pts.
+#' @param inherit.blank Should this element inherit the existence of an
+#' element_blank among its parents?
@hadley

hadley Sep 17, 2016

Owner

Can you expand on this a little bit? I assume if it's TRUE the "blank-ness" will be inherited, otherwise it will skip the parent and look at the grand parent?

R/theme.r
-
+ if (is.null(e2) || inherits(e1, "element_blank")) return(e1)
+ # If e1 is NULL inherit everything from e2
+ if (is.null(e1)) return(e2)
# If e1 is NULL, or if e2 is element_blank, inherit everything from e2
@hadley

hadley Sep 17, 2016

Owner

This comment needs updating too, right?

thomasp85 self-assigned this Sep 17, 2016

thomasp85 added some commits Sep 20, 2016

@thomasp85 thomasp85 Set all replacements to complete so inherit.blank is true fdbe5d0
@thomasp85 thomasp85 Check value of inherit.blank
30e5493
@thomasp85 thomasp85 Test all exported themes have inherit.blank = TRUE elements
ea9b57e
Contributor

Katiedaisey commented Sep 20, 2016 edited

Edit: Just saw you updated your repo, but the following has not changed. Can you confirm my first example with your local branch?

I like this idea. It would prevent the recursive checking that #1581 requires, but your example doesn't seem to work for me.

ggplot(mtcars, aes(wt, mpg)) + geom_point() + theme_minimal() + theme(line = element_blank(), axis.line.y = element_line())

rplot

When I examine the plot theme, it seems line = element_blank() rewrites element_line() and so axis.line.y calls all NULLS. Redoing this with theme(axis.line = element_blank(), axis.line.y = element_line() also results in all NULLS.

When I try p + theme_minimal() + theme(axis.line = element_blank(), axis.line.y = element_line(color = "red")), I get:

rplot04

Examining the plot, I see axis.line.y has color = "red" but isn't printing.

$ axis.line.y          :List of 4
  ..$ colour  : chr "red"
  ..$ size    : NULL
  ..$ linetype: NULL
  ..$ lineend : NULL
  ..- attr(*, "class")= chr [1:2] "element_line" "element"

Also, currently theme_classic() doesn't inherit correctly.
rplot01

Collaborator

thomasp85 commented Sep 20, 2016

This has been fixed I think - truly with the latest version

Collaborator

thomasp85 commented Sep 20, 2016

I cannot confirm any problems and all changes has been pushed - can you try again and make sure you create the plot object from scratch

Owner

hadley commented Sep 20, 2016

LGTM

Contributor

Katiedaisey commented Sep 20, 2016

@thomasp85 Sorry, I previewed the comment but apparently never posted it. Yes, I've found no problems with the last update!

@thomasp85 thomasp85 merged commit 32a2c27 into tidyverse:master Sep 21, 2016

3 checks passed

codecov/patch 100% of diff hit (target 64.69%)
Details
codecov/project 66.13% (+1.44%) compared to 06037a9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment