-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Geom aesthetics based on theme #5833
base: main
Are you sure you want to change the base?
Conversation
Update: added text parameters to devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
ggplot(mtcars, aes(wt, mpg, label = rownames(mtcars))) +
geom_text() +
theme_gray(base_family = "Montserrat", base_size = 22) Created on 2024-04-17 with reprex v2.1.0 |
This is in a useable state now, with almost all geoms completing colour/fill/linewidth/family/(font)size defaults from the theme. What isn't done up to this point:
ggplot2/tests/testthat/test-theme.R Line 224 in 9243e2f
Aside from these points, I'd be happy for any feedback on the direction, so I'm un-WIP-ping this PR. |
Merge branch 'main' into theme_geom_defaults # Conflicts: # R/geom-.R # R/geom-sf.R
Small update:
devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2
nc <- sf::st_read(system.file("shape/nc.shp", package = "sf"), quiet = TRUE)
ggplot(nc) +
geom_sf() +
theme(
geom = element_geom(ink = "forestgreen")
) Created on 2024-04-29 with reprex v2.1.0 |
I've added devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2
ggplot(mpg, aes(class, hwy)) +
geom_boxplot() +
theme(geom = element_geom(
pointsize = 1, pointshape = 3,
ink = "red", paper = "grey80"
)) Created on 2024-04-30 with reprex v2.1.0 I think this is now ready for proper review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from my one comment this generally looks good. I'm slightly unsure about the need for the "paper" colour. The fact that it is always white in ggplot2 makes me feel like it is wasted on encoding that. Do you have examples of extension themes where it would be set to something else and that would lead to good defaults with all the mixing?
Aside, I'd really like @clauswilke and @yutannihilation to have a look at this as they were involved in the superseded PR
Co-authored-by: Thomas Lin Pedersen <[email protected]>
The It is very hard to read out these boxplots: devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2
# Very simplified dark theme
dark_theme <- theme(
plot.background = element_rect(fill = "black"),
panel.background = element_rect(fill = "grey10"),
axis.text = element_text(colour = "white"),
axis.ticks = element_line(colour = "white"),
panel.grid = element_line(colour = "grey5"),
axis.title = element_text(colour = "white")
)
ggplot(mpg, aes(class, displ)) +
geom_boxplot() +
dark_theme But it is way easier when you swap light/dark in ink/paper: last_plot() + theme(geom = element_geom(ink = "white", paper = "grey10")) Created on 2024-04-30 with reprex v2.1.0 |
Addional idea that popped up, but probably best for a different PR; |
I hope I can have time to take a look! By the way, which PR do you mean? I might get lost. |
Thanks! The previous PR for tackling this issue is #2749. |
Oh, thanks. I can't remember how I involved in #2749, so I was wondering if there was a different one. But, that's fine. I'll try to get the context...! |
TODO: Taken from #5886:
|
As in the issue that Hiroaki linked, there might be some revdep fallout for packages that want to compare values from the default aesthetics. We can consider doing a proper revdepcheck to assess the scope of the problem. |
Yeah, while the linked one is simply my fault, I was wondering whether it can be any problem or not to lose access to the actual values of |
I agree, but I think this is a case of not being able to have a cake and eat it. Here I think the pro's outweigh the con's |
result of revue-checks have been pushed. It's north of 200 breakages so definitely a large impact regardless of the usefulness of this. Can you go through the failures and see if there are aspects of this PR that can alleviate some of the issues. Once we are certain with the implementation and have an overview over which packages needs fixing upstream we can merge it in and provide PRs to these |
@@ -437,14 +438,14 @@ Layer <- ggproto("Layer", NULL, | |||
self$position$compute_layer(data, params, layout) | |||
}, | |||
|
|||
compute_geom_2 = function(self, data) { | |||
compute_geom_2 = function(self, data, theme) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this is one of the common errors in the revdep check.
Error in compute_geom_2(..., self = self) :
argument "theme" is missing, with no default
Calls: <Anonymous> ... use_defaults -> eval_from_theme -> %||% -> calc_element
Execution halted
It seems many package use compute_geom_2(d)
directly. So, probably theme
argument of compute_geom_2()
should have the default value of NULL
.
https://github.com/search?q=org%3Acran+compute_geom_2&type=code
This PR aims to partially fix #2239 and is intended to replace #2749.
It is a work in progress as a few things might need to happen before this is viable (more at the end about this).
The main gist is that there is a
from_theme()
function that works in a similar fashion toafter_stat()
/after_scale()
/stage()
.During
Geom$use_defaults()
, this will be used for masked evaluation from a new theme element.See example below for a quick demo:
It also works from the geom defaults, but this isn't implemented in any geom yet, as a few geoms (notably
geom_sf()
) are recalcitrant to this method.Created on 2024-04-08 with reprex v2.1.0
There are a few topics for discussion that mostly mirror those in #2749.
element_geom()
areink
,paper
andaccent
. You can think ofink
andpaper
as foreground and background respectively and could be renamedfg
andbg
if that is deemed more intuitive.colour_1
,colour_2
,fill_1
,fill_2
etc as discussed in Allow default geom aesthetics to be set from theme #2749, is becausefrom_theme()
could allow you to mix colours on the spot and you wouldn't need a large number of colours to account for all the different grayscale defaults in the geoms. See also next point.GeomRect
:geom_sf()
performs typicalGeom
tasks (like setting defaults) in thesf_grob()
and would require a bit of refactoring to wire this geom up directly.