-
Notifications
You must be signed in to change notification settings - Fork 632
Bug fix. Account for multi_line=FALSE when facet rows/cols are missing. Closes #2341. #2373
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
Conversation
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
Lines 937 to 941 in 3cf17c0
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.
R/ggplotly.R
Outdated
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.
Similar recommendation as https://github.com/plotly/plotly.R/pull/2373/files#r1742335996
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.
Thx for the review and the suggestions. Just committed both suggestions.
Thank you for the detailed explanation and contribution! This is looking very close to mergeable, I just had a couple stylistic suggestions
kwilson62
commented
Sep 6, 2024
Looking forward to this getting fixed, currently experiencing the issue with "expression(list())"
kwilson62
commented
Sep 6, 2024
@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?
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
@kwilson62 Will have a look if I find the time. But first let's finish this PR. (:
Uh oh!
There was an error while loading. Please reload this page.
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:
Created on 2024年08月24日 with reprex v2.1.1