Improve HTML5 compliance #2523
Improve HTML5 compliance #2523
Conversation
|
@maelle this needs another review because it substantially expanded in scope. I think we discussed doing this before (but I decided it would be too much work?), but we're now telling pandoc to generate html5 output. This changes the generated HTML so there's some risk that we are no longer transforming it correctly, and because our snapshot tests tend to use canned HTML, it might look like everything is ok even though it's no longer translating the new HTML. I think the biggest culprit is likely to be the change from |
| classes <- strsplit(x$icon, " ")[[1]] | ||
| icon_classes <- classes[grepl("-", classes)] | ||
| iconset <- purrr::map_chr(strsplit(icon_classes, "-"), 1) | ||
| class <- paste0(unique(c(iconset, classes)), collapse = " ") |
There was a problem hiding this comment.
I just use it everywhere so I don't have to remember
R/tweak-tabset.R
Outdated
| @@ -81,7 +81,7 @@ tablist_title <- function(tab) { | |||
|
|
|||
| # Add content of a tab to a tabset | |||
| tablist_content <- function(tab, content, parent_id, fade) { | |||
| id <- section_id(tab) | |||
| id <- xml2::xml_attr(tab, "id") | |||
There was a problem hiding this comment.
if id is an attribute we often ask why not have a helper? today I saw https://github.com/lionel-/codegrip/blob/82d164ed91ad819587796a4053b1df5b0385c677/R/ast.R#L51, it looked nice
There was a problem hiding this comment.
IMO that would generally conceal the purpose of the code more than it reveals it. section_id() was only need before because it was relatively complicated to figure out the id.
| @@ -20,7 +20,7 @@ <h1>{{{pagetitle}}}</h1> | |||
| </main> | |||
|
|
|||
| <aside class="col-md-3"> | |||
| <nav id="toc"> | |||
| <nav id="toc" aria-label="Table of contents"> | |||
There was a problem hiding this comment.
when I switch from R scripts to HTML files in the diff, the first equal sign I see without space before and after makes me jump!
There was a problem hiding this comment.
Hahaha. Also this should be translated too!
| { | ||
| "path": ["/dev/index.html"], | ||
| "id": ["testpackage"], | ||
| "id": [null], |
R/build-articles.R
Outdated
| @@ -279,12 +279,14 @@ build_article <- function(name, | |||
| # Override defaults & values supplied in metadata | |||
| options <- list( | |||
| template = template$path, | |||
| self_contained = FALSE | |||
| self_contained = FALSE, | |||
| pandoc_args = c("-t", "html5") | |||
There was a problem hiding this comment.
About this html5 format, doing it like this will work, but only because pandoc accept two --to arguments, and use the last one. This could be fragile.
Moreover, doing this with an argument like this does not change the internal knitr option that store the output format for some processing in knitr context (knitr::pandoc_to())
So, as you are calling
output <- rmarkdown::default_output_format(input_path) maybe this is safer to overwrite the to information there
output$pandoc$to <- "html5"
This way you are really overwriting the format to change the pandoc to value.
|
I found a bunch more code that I need to update, and I'm beginning to think that switching pandoc to produce isn't a good idea, because I think there's substantial risk of it breaking BS3 sites (I don't really want to mess with the BS3 templates, but I also don't want to maintain two versions of all the tweak scripts, and we have relatively little test coverage of BS3 now). So I'm going to roll back to just the first two commits here, and start the deprecation for BS3 sites. Then we can try again after BS3 is gone. |
Fixes #2369
Remaining issues:
<form>element must have a submit button: would be nice to fix this, but I don't see how.