-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
WIP: Cleanup legend code #5512
WIP: Cleanup legend code #5512
Conversation
Merge branch 'main' into cleanup_legend_code # Conflicts: # R/guide-legend.R
This reverts commit 148006f.
e1$size <- (e2$size %||% rel(1)) * unclass(e1$size) | ||
} | ||
|
||
# Calculate relative linewidth | ||
if (is.rel(e1$linewidth)) { | ||
e1$linewidth <- e2$linewidth * unclass(e1$linewidth) | ||
e1$linewidth <- (e2$linewidth %||% rel(1)) * unclass(e1$linewidth) |
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.
If e2
has NULL
size/linewidth, this used to result in numeric(0)
as size/linewidth. {grid} cannot deal with 0-length gpar settings.
OK, so the main idea here is that we can abstract away most of the theme-guide interaction fuss, by just having the guide carry an internal theme that can be added to the plot's theme. However, this is currently also fussy due to the following behaviours of
devtools::load_all("~/packages/ggplot2")
#> ℹ Loading ggplot2
add_theme(
theme(text = element_blank()), # plot theme
theme(text = element_text(inherit.blank = TRUE)) # guide theme
)$text |> class()
#> [1] "element_text" "element"
add_theme(
theme(text = element_text()), # plot theme
theme(text = NULL) # guide theme
)$text
#> NULL
add_theme(
theme(text = ggtext::element_markdown()), # plot theme
theme(text = element_text(hjust = 0.5)) # guide theme
)
#> Error:
#> ! Problem merging the `text` theme element
#> Caused by error in `merge_element()` at ggplot2/R/theme.R:540:7:
#> ! Only elements of the same class can be merged Created on 2023-11-27 with reprex v2.0.2 For now, I've worked around this by making a custom theme merging function, but maybe there are smarter solutions that I couldn't come up with. |
Closing this in favour of #5554 |
The legend code is lettered with all sorts of fussing about theme/element settings. This PR aims to to limit that clutter and make the reasoning about guide/theme interaction much clearer.
Essentially, it implements #5348 internally and passes around an
internal_theme
object that will get merged with the plot's theme. This has the following advantages:Guide$setup_elements()
becomes the pure place to break theme inheritance without overriding guide-level user input.Guide$override_elements()
becomes the place to override guide-level user input.It made the concession that expression vectors are no longer right-aligned by default (see #4814), as it seemed just very out-of-place to me. Please let me know if this should be preserved.
Current blockers:
merge_element()
guide_colourbar()
#5455 gives a changed visual test that is changed and should probably be adressed before merging this.