Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Open
trekonom wants to merge 3 commits into plotly:master
base: master
Choose a base branch
Loading
from trekonom:issue-2341

Conversation

Copy link
Contributor

@trekonom trekonom commented Aug 5, 2024
edited
Loading

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

Copy link
Contributor Author

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

plotly.R/R/ggplotly.R

Lines 937 to 941 in 3cf17c0

row_txt <- paste(
plot$facet$params$labeller(
lay[names(plot$facet$params$rows)]
), collapse = br()
)

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.

kwilson62 reacted with heart emoji

R/ggplotly.R Outdated
Comment on lines 943 to 952
row_txt <- if (!length(names(plot$facet$params$rows)) == 0) {
paste(
plot$facet$params$labeller(
lay[names(plot$facet$params$rows)]
),
collapse = br()
)
} else {
""
}
Copy link
Collaborator

@cpsievert cpsievert Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@trekonom trekonom Sep 6, 2024

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.

Copy link
Collaborator

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

kwilson62 reacted with thumbs up emoji

Copy link

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

Copy link

@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?

trekonom and others added 2 commits September 6, 2024 08:58
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Copy link
Contributor Author

trekonom commented Sep 6, 2024

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

kwilson62 reacted with heart emoji

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cpsievert cpsievert cpsievert left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /