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

Difficulty in using class.output option with collapse=T #2172

Closed
benwhalley opened this issue Sep 20, 2022 · 8 comments · Fixed by #2212
Closed

Difficulty in using class.output option with collapse=T #2172

benwhalley opened this issue Sep 20, 2022 · 8 comments · Fixed by #2212

Comments

@benwhalley
Copy link

benwhalley commented Sep 20, 2022

This reopens this issue: #1906

These screenshots illustrate the problem. The only thing that changes is the value for collapse chunk option:

With collapse = F

Screenshot 2022-09-20 at 14 35 28

With collapse = T

Screenshot 2022-09-20 at 14 35 11


Minimal example is here: https://gist.github.com/benwhalley/99b2ebf4b58605e0cec35727793190f6

I can see there was some previous discussion about expected behaviour, but the problem problem is that if collapse=T then no distinction is made between the R code being echoed and the resulting output (and no different css is applied, so results can't be styled differently to commands).

I can see that for the second code chunk it's not obvious which classes to output, but at the very least allowing the author to know that something is a result rather than the command that produced it would be very useful.

In my case, I could just set collapse=F but this results in very fragmented commands and output which is less good for explanatory purposes.

@cderv
Copy link
Collaborator

cderv commented Sep 21, 2022

The purpose of collapse = TRUE is to collapse all the source and output blocks from one code chunk into a single bloc.
This means on HTML side it will be in one <pre><code> block by design. This will also mean that syntax highlighting will apply on the all block usually.

the problem problem is that if collapse=T then no distinction is made between the R code being echoed and the resulting output (and no different css is applied, so results can't be styled differently to commands).

Usually when setting collapse = TRUE as a global option, one want also to set for example the comment option to help differentiate code and result as you explain.

knitr::opts_chunk$set(
  collapse = TRUE,
  comment = "#>"
)

image

See for example R4DS book.

A prompt could be added for R code line but this is not good for highlighting

knitr::opts_chunk$set(
  collapse = TRUE,
  prompt = TRUE
)

image

this results in very fragmented commands and output which is less good for explanatory purposes.

You could also look at tweaking the CSS for leaving less space between the source block and result block maybe.

I don't really know what we could do to change this. Do you have suggestion based on the current structure of HTML produce when collapse = TRUE ? What would you expect at results exactly ? Is it really collapsing or just a different CSS styling to closer match results chunks with their source code maybe ?

@benwhalley
Copy link
Author

Ideally the default would be that this:

mean(mtcars$mpg)

Would produce something like:

<pre>
<span class="code">
mean(mtcars$mpg)
</span>
<span class="output">
[1] 20.09062
</span>
</pre>

This way you could style the code and output differently.

The problem with having a prefix for results or messages (e.g. #> or ##) is that it makes it hard to copy and paste. For code especially, this really confuses students/beginners who often copy the prompt. I know this seems a silly error, but it is incredibly common... I teach R and stats to 1st and 2nd year UGs and see this every year despite help from staff.

If you also then set output classes it would be possible to have:

mean(mtcars$mpg)
warning("hello")

Would produce something like:

<pre>
<span class="code">
mean(mtcars$mpg)
</span>
<span class="output bg-success">
[1] 20.09062
</span>
<span class="code">
warning("hello")
</span>
<span class="output bg-warning">
Hello
</span>
</pre>

@benwhalley
Copy link
Author

Just to add, an example here: https://codepen.io/benwhalley/pen/jOxLEoo

I realised that spans within pre tags wouldn't allow a background color, so divs might be better. Either would be better than current behaviour thought I think.

@cderv
Copy link
Collaborator

cderv commented Sep 21, 2022

One thing is that knitr does not produce the HTML structure of the code. knitr will produce markdown content, to be converted to HTML or PDF or else, by Pandoc. https://bookdown.org/yihui/rmarkdown-cookbook/rmarkdown-process.html

So when setting collapse = TRUE, this chunk

```{r}
mean(mtcars$mpg)
```

will produce this markdown in the intermediate .md file

```r
mean(mtcars$mpg)
## [1] 20.09062
```

(by default, comment = "##" which allows to differentiate code from result in the output, but you removed it )

It will be converted to HTML by Pandoc to <pre><code> structure, and then the HTML content structure will be created by syntax highlighter used (by default highlight..js, but could be Pandoc own system) - https://pkgs.rstudio.com/rmarkdown/reference/html_document.html#highlighting-1

So there is no control over what you expect unfortunately.

Regarding <span> or <div> inside <pre>, I don't think it is that common. <pre><code> is expected by most of the tools to detect code part in a page, especially for highlighting (For example highlight.js).

Even if we could come up with a new way to change the structure (which downlit R package is doing), we could also need to come up or tweak the syntax highlighter.

Overall it is not that obvious, and we won't probably change this in knitr at this is not the correct level. It would need to be in one of the syntax highligther but they usually are static analysis only and don't know results or it would need to be a specific R tools (like downlit) for this specific usage, that would use special knitr hooks to output directly a structure like you want. Problem of syntax highlighting would need to be handled specifically too.

The problem with having a prefix for results or messages (e.g. #> or ##) is that it makes it hard to copy and paste.

The comment part is here by default so that when copying the code chunk content from the collapse block, this is still valid R code. R expression does get evaluated when paster, while results being commented they are ignored.

For code especially, this really confuses students/beginners who often copy the prompt. I know this seems a silly error, but it is incredibly common... I teach R and stats to 1st and 2nd year UGs and see this every year despite help from staff.

Just to shere, there are some tools to add a copy button in chunks like xaringanExtra::use_clipboard()`or https://github.com/RLesur/klippy

This exists in bookdown too but we still need to make it clever to ignore prompt and results rstudio/bookdown#1072

So we could probably add to rmarkdown as default feature to add a clever copy button that would keep only the code to avoid copy issues.

I hope this clarify the topic and challenge about this.

@benwhalley
Copy link
Author

I understand that the processing is done by pandoc etc, although I guess that doesn't preclude using markdown syntax to add the classes to multiple elements.

However I see that what I'm suggesting is actually different from the collapse option. What I'd like is for each part of the output to be created individually (akin to collapse=F) but where all of the code is echoed first, before any of the output, and for that echoed code to have it's own block element which can have a class attached.

One way to do this would be to amend the echo option. It could be:

  • TRUE (current behaviour)
  • FALSE (current behaviour)
  • "inline" (same as TRUE)
  • "before" (new behaviour, puts all echoed code before any of the output

This would be backwards compatible, but avoid the need to set collapse=T if you want all of the output after all of the code .

@cderv
Copy link
Collaborator

cderv commented Sep 21, 2022

You may be looking for results = "hold"

image

Is it closer to what you are looking for ?

@dmurdoch
Copy link
Contributor

I've just noticed that collapse = TRUE also messes up handling of htmlwidgets output. I saw it first in rgl, but it's also present in leaflet. For example, with collapse = TRUE this code chunk

```{r collapse = TRUE, comment = "#>"}
library(leaflet)
leaflet() %>% addTiles()

# This is some more code after the widget
123
```

renders like this in an html_vignette:
Screen Shot 2023-01-25 at 4 12 51 PM

Notice that the final comment and expression aren't typeset as code, they are typeset as regular text. The same problem happens in html_document. The Markdown produced by rmarkdown::render() looks like this:

```r
library(leaflet)
leaflet() %>% addTiles()
```

```{=html}
<div class="leaflet html-widget html-fill-item-overflow-hidden html-fill-item" id="htmlwidget-067772b9251e7c9c20ad" style="width:288px;height:288px;"></div>
<script type="application/json" data-for="htmlwidget-067772b9251e7c9c20ad">{"x":{"options":{"crs":{"crsClass":"L.CRS.EPSG3857","code":null,"proj4def":null,"projectedBounds":null,"options":{}}},"calls":[{"method":"addTiles","args":["https://{s}.tile.openstreetmap.org/{z}/{x}/{y}.png",null,null,{"minZoom":0,"maxZoom":18,"tileSize":256,"subdomains":"abc","errorTileUrl":"","tms":false,"noWrap":false,"zoomOffset":0,"zoomReverse":false,"opacity":1,"zIndex":1,"detectRetina":false,"attribution":"&copy; <a href=\"https://openstreetmap.org\">OpenStreetMap<\/a> contributors, <a href=\"https://creativecommons.org/licenses/by-sa/2.0/\">CC-BY-SA<\/a>"}]}]},"evals":[],"jsHooks":[]}</script>

# This is some more code after the widget
123
#> [1] 123
```

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants