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

Allow to set the lang attribute, will close #82 #185

Merged
merged 2 commits into from
Oct 8, 2020

Conversation

ColinFay
Copy link
Contributor

@ColinFay ColinFay commented Oct 2, 2020

No description provided.

@cpsievert
Copy link
Collaborator

cpsievert commented Oct 2, 2020

Nice, thanks! We'd also appreciate a NEWS item and a unit test.

@wch
Copy link
Collaborator

wch commented Oct 2, 2020

It seems to me that the lang should be a property of the object being saved, rather than an argument to save_html(). On the other hand, there's not an obvious place to store that property.

We currently have a PR for this in Shiny, where we're adding a lang attribute to the top-level tag in the ui object. However, that approach may not make as much sense for the general case that this PR is trying to address. rstudio/shiny#2920

@yihui, @cderv, What cases do you have in mind where save_html() is called from?

@ColinFay
Copy link
Contributor Author

ColinFay commented Oct 2, 2020

Test and NEWS bullet added ✅

@cderv
Copy link
Contributor

cderv commented Oct 5, 2020

What cases do you have in mind where save_html() is called from?

I don't have a specific case in mind other than exporting some HTML to a file - it is just that I recently understand than setting lang attributes was always a good practice (and even a must-have) for any html file (especially for accessibility with screen reader). It seems good to me that save_html allows to create a html file with a correct lang attribute, as it takes care of creating the necessary <html> for the user. As lang attributes is part of the whole document definition (<html> node), it seems ok to me to have it as a parameter.

I am not sure anyone will provide <html lang="fr"> in the content pass to save_wiget. And even if htmltools::tags$html(lang="fr", ...) is provided in the content, it currently don't play well with the save_html function as it would generate an odd html structure, with a <html> tag in the middle of the document that the browser seems to recognize anyway...

Example of providing `tags$html(lang = "fr", ...)`
library(htmltools)
temp_html <- tempfile(fileext = ".html")
html <-  tags$html(
  lang="fr", 
  tags$head(
    tags$style("h1{color:red;}")
  ),
  tags$body(
    tags$h1("title", id = "title1")
  )
)
save_html(html, temp_html)
xfun::file_string(temp_html)
#> <!DOCTYPE html>
#> <html>
#> <head>
#> <meta charset="utf-8"/>
#> <style>body{background-color:white;}</style>
#> 
#>   <style>h1{color:red;}</style>
#> </head>
#> <html lang="fr">
#>   <body>
#>     <h1 id="title1">title</h1>
#>   </body>
#> </html>
#> </html>
unlink(temp_html)

Created on 2020-10-05 by the reprex package (v0.3.0.9001)

If we don't want to have specific attributes as function parameters, we could make it more generic and the function could take a list of html.attr to the <html> node created.

save_html(html, file = "test.html", html.attr = list(lang="fr"))

@wch wch merged commit 5d42d84 into rstudio:master Oct 8, 2020
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.

4 participants