Unexpected change in styling for code blocks

grimbough commented 3 years ago

I've just noticed that some of the styling seems to have changed for vignettes in the devel branch. Most noticeably the background on code blocks seems to have disappeared (or turned white) e.g. release vs devel

It doesn't look like we've introduced any changes recently, so either this happened a while ago and I haven't realised or there's some upstream change. However, normally when a CRAN package does something new it affects both release and devel on BioC.

I'll take a look as I don't like the new style, but I wanted to note it here incase someone else already new about this.

aoles commented 3 years ago

Thanks for looking into it! There seem to be a slight difference in paragraph column width as well + the lack of Abstract indentation. Seems to have sth to do with the newer knitr and rmarkdown versions in devel: 1.31 vs. 1.30 and 2.7 vs. 2.6, respectively. Cheers!

grimbough commented 3 years ago

Thanks for the tips. I hadn't checked the knitr & markdown versions. I guess we can expect the release vignettes to be affected sometime in the future when the build machine updates.

This also has a pretty horrendous effect on the narrow screen rendering:

grimbough commented 3 years ago

I think it was https://github.com/rstudio/rmarkdown/pull/1706 that introduced the changes.

A change from Bootstrap 3 to 4 would explain the layout changes so col-xs-12 would become col-sm-12 and lots of the CSS wouldn't apply.

This line seems to be responsible for the code block background color:

https://github.com/rstudio/rmarkdown/blob/61db7a9f5b18d77609877e3a3a039758b17d554a/R/html_dependencies.R#L77

grimbough commented 3 years ago

Not found and easy solution yet, but the overlapping TOC seems to affect even a plan rmarkdown::html_document(), so it's not just BiocStyle. I've submitted an issue. We'll see how that goes, but I propose not making any changes until the rmarkdown authors have commented on that.

aoles commented 3 years ago

Awesome, thanks a lot for digging into it! Yeah, let's wait for some feedback on the upstream rmarkdown issue. Cheers!

cderv commented 3 years ago

Hi everyone.

Issue is in rmarkdown indeed with a small glitch when bs4 support was introduced. Thanks for the report @grimbough !

Issue is in the HTML template itself and it should be fixed by https://github.com/rstudio/rmarkdown/pull/2072

Sorry for the trouble

grimbough commented 3 years ago

Thanks a lot @cderv ! That seems to have fixed the TOC layout issues.

I've hopefully addressed the background styling of the code output blocks in #87

The different paragraph widths etc are because the main body div is now a Bootstrap row rather than row-fluid (https://github.com/rstudio/rmarkdown/blob/61db7a9f5b18d77609877e3a3a039758b17d554a/inst/rmd/h/default.html#L448). ~~I'll admit I'm a bit lost as to when we use the template.html distributed in BiocStyle vs the one the in rmarkdown, and~~ I'm not sure I have an opinion on whether we care about this change.

EDIT: On closer inspection I see we use readUTF8(system.file("rmd", "h", "default.html", package = "rmarkdown")). Is the file at inst/resources/html/tempalte.html ever used?

cderv commented 3 years ago

I'll admit I'm a bit lost as to when we use the template.html distributed in BiocStyle vs the one the in rmarkdown,

It seems you are using a "hack" where you use the default template from rmarkdown then adapting to your template by modifiying the former "on the run". This requires you to adapt your processing each time there is a change in a rmarkdown template because you are doing that dynamically at each rendering (in comparaison to have a fixed template shipped with your package that you would update and patch with your package version, based on the changes in rmarkdown). I understand that from BiocStyle::html_document.

I think it is why if you already patched the CSS rule, you may need to do it differently because it is now introduced in head as a script tag introduced by Boostrap 3 dependency (meaning it won't be there with other boostrap version) instead of in the template when theme variable is active. Your fix seems ok then.

But about this background color, you get this rule applied because there is not class applied on the code source block meaning <pre> will be without class. This is needed in default rmarkdown to have white background on the result code block. So if you don't wan't that you need indeed to remove or overwrite the rule. (or apply classes on your block. (just sharing that pandoc as a way to apply a default class on indented code block see --indented-code-classes)

On closer inspection I see we use readUTF8(system.file("rmd", "h", "default.html", package = "rmarkdown")). Is the file at inst/resources/html/tempalte.html ever used?

This file system.file("rmd", "h", "default.html", package = "rmarkdown") is the default template used by rmarkdown internally. You are using it and patch it.

The file inst/resources/html/template.html is internal to your package. It won't be used by rmarkdown itself, and it seems it was used by BiocStyle in previous versions https://github.com/Bioconductor/BiocStyle/blob/5a9b7b342ddc54585612ba4de09bc2d340401b3b/R/html_document.R#L44 Things seems to have change but those now unused resources files not removed from the source.

The different paragraph widths etc are because the main body div is now a Bootstrap row rather than row-fluid

When I saw that change, I was not sure of the implication. You can open an issue in rmarkdown so that we can discuss this change. It could be a regression we may fixed - we'll have to see the implication with new bootstrap 4 support.

cderv commented 3 years ago

There seem to be a slight difference in paragraph column width as well

About this, html_document template was using an old Bootstrap 2 class. We fixed that by using the correct .row class and this lead to make the boostrap 3 css rule apply:

.row {
    margin-right: -15px;
    margin-left: -15px;
}

I believe you can revert this by unsetting this rule if you don't like that. it is set by boostrap not rmarkdown directly.

the lack of Abstract indentation.

Seems like the content of the abstract section is not encapsulated in <p> tag anymore:

devel

<h4 class="abstract">Abstract</h4>
Instructions on enabling <em>Bioconductor</em> style in R Markdown vignettes.
<h4 class="package">Package</h4>
<p>BiocStyle 2.19.1</p>

release

<h4 class="abstract">Abstract</h4>
<p>Instructions on enabling <em>Bioconductor</em> style in R Markdown vignettes.</p>
<h4 class="package">Package</h4>
<p>BiocStyle 2.18.1</p>

Which leads to the <p> CSS not applying.

Not sure what changed on our side, but you are not setting the p tag in the template I think https://github.com/Bioconductor/BiocStyle/blob/53d1219a4a55d7a464fdfef4259dd59856aca8b8/R/html_document.R#L160

Maybe Pandoc is not passing / using the variable the same way. I don't know.

grimbough commented 3 years ago

Thanks for the followup information - that's really helpful.

Bioconductor / BiocStyle

Unexpected change in styling for code blocks #86