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

WIP: Cleanup legend code #5512

Closed
wants to merge 20 commits into from
Closed

Conversation

teunbrand
Copy link
Collaborator

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:

  • Reduces clutter
  • 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:

Comment on lines +777 to +782
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)
Copy link
Collaborator Author

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.

@teunbrand
Copy link
Collaborator Author

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 add_theme().`

  1. it happily merges text over blank, even if inherit.blank = TRUE.
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"
  1. It lets a proper element be overruled by NULL elements
add_theme(
  theme(text = element_text()), # plot theme
  theme(text = NULL) # guide theme
)$text
#> NULL
  1. We cannot merge simpler elements into richer elements.
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.

@teunbrand teunbrand mentioned this pull request Dec 1, 2023
@teunbrand
Copy link
Collaborator Author

Closing this in favour of #5554

@teunbrand teunbrand closed this Dec 1, 2023
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

Successfully merging this pull request may close these issues.

None yet

1 participant