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

Feature request: easy functions should work on more specific theme elements #75

Open
davidhodge931 opened this issue Feb 14, 2024 · 8 comments

Comments

@davidhodge931
Copy link

davidhodge931 commented Feb 14, 2024

Lots of theme elements might have more specific theme elements specified than that which the ggeasy function affects.

In these cases, the ggeasy function does not work - and users will often be confused.

Ideally it would just work regardless of the theme elements already specified - as the user just wants to remove them all.

An idea is to have an argument within these easy functions to be as specific as possible.

I'm thinking a specific = TRUE argument - or a broad = FALSE argument.

Not sure whether it would default to being specific or not. That would depend if your priority was teaching users, or just doing what the user wants to do. I'd vote for just working being the priority. I know how ggplot2 theme statements work, but I prefer to just use ggeasy.

For example, with easy_remove_y_axis(what = "text", specific = TRUE) or equivalent argument, it would add:

theme(axis.text.x.bottom = element_blank(), axis.text.x.top = element_blank())

library(tidyverse)
library(palmerpenguins)

penguins |> 
  ggplot() +
  geom_point(aes(x = flipper_length_mm, body_mass_g)) +
  hrbrthemes::theme_ipsum() +
  ggeasy::easy_remove_axes()
#> Warning: Removed 2 rows containing missing values or values outside the scale range
#> (`geom_point()`).
#> Warning in grid.Call(C_stringMetric, as.graphicsAnnot(x$label)): font family
#> not found in Windows font database

#> Warning in grid.Call(C_stringMetric, as.graphicsAnnot(x$label)): font family
#> not found in Windows font database
#> Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
#> font family not found in Windows font database

#> Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
#> font family not found in Windows font database

#> Warning in grid.Call.graphics(C_text, as.graphicsAnnot(x$label), x$x, x$y, :
#> font family not found in Windows font database

Created on 2024-02-15 with reprex v2.1.0

@davidhodge931 davidhodge931 changed the title easy functions do not work on more specific theme elements Feature request: easy functions should work on more specific theme elements Feb 14, 2024
@jonocarroll
Copy link
Owner

This might actually be an issue with the implemented theme or ggplot2 itself - ggplot2 theme elements inherit via a hierarchy which is why we're able to use e.g. axis.text = element_blank() to remove both x and y axes. In the case of hrbrthemes, being a little more specific does work

library(ggplot2)
library(ggeasy)

p <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  hrbrthemes::theme_ipsum() 
p

# these doesn't remove axes
p + ggeasy::easy_remove_axes()

p + theme(axis.text = element_blank())

# but these do
p + ggeasy::easy_remove_x_axis()

p + ggeasy::easy_remove_y_axis()

I think this also has to do with re-defining theme elements. If I manually change a specific theme element, I need to be specific when removing it

library(ggplot2)
p2 <- ggplot(mtcars, aes(wt, mpg)) +
  geom_point() + 
  theme_classic()

# if a theme element is set manually, the inheritance doesn't work
p2 + theme(axis.line.x = element_line(color = "red"))  

# the red x-axis remains, even though "everything" is removed
p2 + theme(axis.line.x = element_line(color = "red")) + ggeasy::easy_remove_axes()

# you need to specifically reset that element
p2 + theme(axis.line.x = element_line(color = "red")) + ggeasy::easy_remove_x_axis()

In terms of the inheritance working properly, it seems the "right" way is that new theme elements should be registered and their place in the element tree defined. See: https://ggplot2.tidyverse.org/reference/register_theme_elements.html

But I can't find a single package that does it properly. {ggside} was the closest I could find https://github.com/jtlandis/ggside/blob/main/R/zzz.R -- it does do the registering, but it doesn't define any elements that inherit from a default one.

I might reach out to the community to better understand why setting e.g. axis.text doesn't affect a previously modified axis.text.x in case there's something I'm not appreciating about it. If what I'm seeing is not the expected behaviour then fixing that should propagate to more specific elements like axis.text.x.bottom.

@jonocarroll
Copy link
Owner

"the most specific theme element wins" appears to be expected behaviour, now that I see this video https://resources.rstudio.com/resources/rstudioglobal-2021/always-look-on-the-bright-side-of-plots/ (around 18:10 mark).

I might have to play around with detecting already set elements and inheritances. It would be nice if easy_remove_axes() worked when axis.line.x is set. One workaround might just be to make the "both" route call easy_remove_x_axis() and easy_remove_y_axis() which do work as expected. I'll need to see how that might work with the teach argument, too.

@davidhodge931
Copy link
Author

davidhodge931 commented Feb 16, 2024

I think almost no-one understands this. That quoted expression is a nice way to describe it.

It affects most of your functions. I reckon a specific = TRUE/FALSE argument would be best, as this method could be used in all functions. Some hierarchies go quite deep too (e.g. axis.line.x.top)

If you default to specific = FALSE, this enables people to start to learn about how most specific element wins - as well as just enables what some people just want to happen in specific = TRUE

@jonocarroll
Copy link
Owner

Part of the problem would be figuring out which more specific elements have been set by the theme, in order to remove those. That gets even trickier if packages define their own with the registration mechanism. We could just loop through all the ones we know about and explicitly remove those, but my heuristic for "is this too complicated for ggeasy" is usually how bad does the teach=TRUE statement look?

It's still true that being specific in the easy_remove... call works, just not the generalised wrappers.

@davidhodge931
Copy link
Author

davidhodge931 commented Feb 16, 2024

I think you just define at the lowest level, and then you over-rule everything

So easy_remove_y_axis(what = c("line", "ticks"), specific = TRUE) does this

theme(
        axis.line.y.left = ggplot2::element_blank(),
        axis.line.y.right = ggplot2::element_blank(),
        axis.ticks.y.left = ggplot2::element_blank(),
        axis.ticks.y.right = ggplot2::element_blank()
)

And easy_remove_axes(what = c("line", "ticks"), specific = TRUE) does this

theme(
        axis.line.x.left = ggplot2::element_blank(),
        axis.line.x.right = ggplot2::element_blank(),
        axis.ticks.x.left = ggplot2::element_blank(),
        axis.ticks.x.right = ggplot2::element_blank()
        axis.line.y.left = ggplot2::element_blank(),
        axis.line.y.right = ggplot2::element_blank(),
        axis.ticks.y.left = ggplot2::element_blank(),
        axis.ticks.y.right = ggplot2::element_blank()
)

The teach statements in specific = TRUE would be bad, but that would still teach users about all the downstream layers required to make sure something definitely happens

@davidhodge931
Copy link
Author

I reckon it would make ggeasy even more useful, if you could be confident that each easy function would just work regardless of the underlying theme (which often people are unaware what's in it anyway - as someone else made it)

@jonocarroll
Copy link
Owner

@teunbrand has an implementation of the recursive application in tidyverse/ggplot2#5694 so I'll add that to {ggeasy}. I will see how well it can be used throughout.

@davidhodge931
Copy link
Author

Awesome function! I'd vote for making the functions default to recursive, so that they always "just work". This is what the user most wants when they add a ggeasy function

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

No branches or pull requests

2 participants