plotly.R icon indicating copy to clipboard operation
plotly.R copied to clipboard

Bug fix. Account for multi_line=FALSE when facet rows/cols are missing. Closes #2341.

Open trekonom opened this issue 1 year ago • 5 comments

This PR proposes a fix to account for multi_line=FALSE in facet labeller functions, thereby closing #2341 .

With the proposed fix, the reprex from #2341 renders correctly without the undesired, additional strip texts for the rows:

library(plotly, warn = FALSE)
#> Loading required package: ggplot2

mtcars$cyl2 <- factor(
  mtcars$cyl,
  labels = c("alpha", "beta", "gamma")
)

ggplot(mtcars, aes(wt, mpg)) +
  geom_point() +
  facet_wrap(. ~ cyl2,
    labeller = label_wrap_gen(
      width = 25, multi_line = FALSE
    )
  )


ggplotly()

Created on 2024-08-24 with reprex v2.1.1

trekonom avatar Aug 05 '24 06:08 trekonom

And here is a more detailed description of the issue and the proposed fix:

The underlying issue is that ggplotly() creates a strip text for facet rows/cols even if these are missing, e.g. in case of facet_wrap a rows text is created in https://github.com/plotly/plotly.R/blob/3cf17c00477e6992cde648c6e711d5b464b04439/R/ggplotly.R#L937-L941 although in this case plot$facets$params has no rows element and hence plot$facets$params$rows is NULL.

This does not result in an issue as long as multi_line=TRUE (which is the default for the label_xxx family of functions) as in this case the label_xxx family of functions will return an empty list which gets "coerced" to an empty string. However, with multi_line=FALSE the returned list is wrapped in expression and as a result gets coerced to a string "expression(list())" giving rise to the issue reported in #2341 .

In case of facet_wrap this can be fixed by creating a rows text only if inherits(plot$facet, "FacetGrid") is TRUE. However, the same issue arises with facet_grid when the rows/cols are missing:

library(plotly, warn=FALSE)
#> Loading required package: ggplot2

g <- ggplot(mtcars, aes(mpg, wt)) +
  geom_point() +
  facet_grid(vs + am ~ .,
    labeller = function(x) label_both(x, multi_line = FALSE)
  )

ggplotly()

Created on 2024-08-05 with reprex v2.1.0

To account for this case the PR adds an if to check that there is anything to label.

trekonom avatar Aug 24 '24 08:08 trekonom

Thank you for the detailed explanation and contribution! This is looking very close to mergeable, I just had a couple stylistic suggestions

cpsievert avatar Sep 03 '24 16:09 cpsievert

Looking forward to this getting fixed, currently experiencing the issue with "expression(list())"

kwilson62 avatar Sep 06 '24 02:09 kwilson62

@trekonom In addition to the bug you've fixed, there's also an issue where strip.position is not working in plotly. Considering this issue has been around for awhile now, as seen in this issue: #2103, and you seem to be the most recent / successful at fixing some of these facet issues, do you think you could take a crack at it?

kwilson62 avatar Sep 06 '24 06:09 kwilson62

@kwilson62 Will have a look if I find the time. But first let's finish this PR. (:

trekonom avatar Sep 06 '24 10:09 trekonom