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

Translate \n to <br /> in fct labels, too #1700

Merged
merged 11 commits into from
Jun 22, 2020
Merged

Translate \n to <br /> in fct labels, too #1700

merged 11 commits into from
Jun 22, 2020

Conversation

salim-b
Copy link
Contributor

@salim-b salim-b commented Feb 11, 2020

Before, this only worked for colums of type character. This PR adds support for columns of type factor.

The linebreak conversion was first introduced in v4.6.0, see also #851

@cpsievert
Copy link
Collaborator

Thanks, could you provide some examples of where this is relevant and also write a few tests?

@salim-b
Copy link
Contributor Author

salim-b commented Apr 23, 2020

Sorry for the delay! I finally found some time to meet your requests.

could you provide some examples of where this is relevant

This is relevant whenever you want to plot a data column of type factor with labels containing line breaks. Currently, the line breaks are only translated to HTML (<br>) when the column is of type character. But factor columns are not only very common but also very convenient to work with (using pkg forcats), i.e. to (re)arrange, extend, split into "buckets" etc.

A very simple example with multiline factor labels

library(magrittr)

iris %>%
    dplyr::mutate(Species =
                      Species %>%
                      forcats::fct_relabel(paste0,
                                           "\n\n(third line)\n(fourth line)")) %>%
    plotly::plot_ly(x = ~Sepal.Length,
                    y = ~Species)
#> No trace type specified:
#>   Based on info supplied, a 'bar' trace seems appropriate.
#>   Read more about this trace type -> https://plot.ly/r/reference/#bar

grafik

Created on 2020-04-23 by the reprex package (v0.3.0)

The same example when the column is converted to type character before plotting

library(magrittr)

iris %>%
  dplyr::mutate(Species =
                  Species %>%
                  forcats::fct_relabel(paste0,
                                       "\n\n(third line)\n(fourth line)") %>%
                  as.character()) %>%
  plotly::plot_ly(x = ~Sepal.Length,
                  y = ~Species)
#> No trace type specified:
#>   Based on info supplied, a 'bar' trace seems appropriate.
#>   Read more about this trace type -> https://plot.ly/r/reference/#bar

grafik

Created on 2020-04-23 by the reprex package (v0.3.0)

and also write a few tests?

I pushed 841e7e9 which adds an additional test to tests/testthat/test-plotly.R. I'm very new to writing tests (or R packages to begin with), so please bear with me :)

@cpsievert
Copy link
Collaborator

Thanks for the update, this is looking good @salim-b!

A couple minor nitpicks:

  • Could you please remove the %>% and the forcats dependency in the tests?
  • Please switch the order in the DESCRIPTION (I'd prefer that all single authors are listed first, then Plotly)

@salim-b
Copy link
Contributor Author

salim-b commented May 21, 2020

  • Could you please remove the %>% and the forcats dependency in the tests?

Sorry, I assumed it would be fine to use forcats since it's listed under Suggests: in DESCRIPTION. I have replaced it now with equivalent base R code.

Regarding %>%: The pipe operator is used throughout this test file. Why can't I use it, too?

  • Please switch the order in the DESCRIPTION (I'd prefer that all single authors are listed first, then Plotly)

Done :)

@salim-b
Copy link
Contributor Author

salim-b commented Jun 22, 2020

@cpsievert I've added the missing news item in d60898a. I'd be glad if you could merge this PR. 🙂

@cpsievert cpsievert merged commit 9a856ae into plotly:master Jun 22, 2020
@cpsievert
Copy link
Collaborator

Thank you!

@salim-b salim-b deleted the wrap-fct-lbls branch June 22, 2020 16:43
chschan pushed a commit to Displayr/plotly that referenced this pull request Jun 22, 2020
* Fix bug in verify_attr. Closes issue plotly#1720.

* Add test to check that ggplotly does not break discrete x-axis in facet panels with only one category.

* Update tests/testthat/test-facet-axis.R

Co-authored-by: Carson Sievert <cpsievert1@gmail.com>

* Add news item.

* Translate `\n` to `<br />` in fct labels, too (plotly#1700)

* Translate `\n` to `<br />` in fct labels, too

Before, this only worked for colums of type character.

* add @salim-b as ctb

* Add test for `translate_linebreaks()`

* remove superfluous char conversion

* change order of authors

* replace forcats code with base R

* add news item

Co-authored-by: Stefan Moog <moogs@gmx.de>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Salim B <salim@posteo.de>
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

2 participants