Closed bencomp closed 5 months ago
tobyhodges
commented
1 year ago I'm afraid I couldn't recreate this on my side, when I was building locally to preview your changes in https://github.com/datacarpentry/openrefine-socialsci/pull/154, @bencomp. All of the boxes looked okay to me. Further investigation is warranted, I think.
bencomp
commented
1 year ago Thanks for checking, @tobyhodges – let me check if I can get more debugging output, or at least see what versions of package I have installed. I can already tell that the same happens when I build the current main branch and that it doesn't matter if I use serve() or build_lesson(), even after reset_site().
So this could very well be related to my installation. And because I did install with Homebrew, I understand if it is out of scope.
Some of the Brewed dependencies (I don't know which are most relevant):
libgit2 1.6.4
libxml2 2.11.2
lua 5.4.5
luajit 2.1.0-beta3-20230430.1
luajit-openresty 2.1-20230410
pandoc 3.1.2
pkg-config 0.29.2_3
r 4.3.0_1
zkamvar
commented
1 year ago This should have been fixed in {sandpaper} 0.12.0 via https://github.com/carpentries/sandpaper/pull/467 and appears to be correct in the live version of the site which was built an hour ago:
I'm not sure what's going on with your installation, locally though. My suggestion would be to rebuild the site from scratch to see if you are running into a cache issue:
sandpaper::build_lesson(rebuild = TRUE)For context, this bug is partially because when we were designing the front end, we did not have an example of a callout block embedded within another callout block (with the exception of the solution blocks) as shown in learners/setup.md#L57-L80:
:::::::::::::::::::::::::::::::::::::::::: prereq
## Software
For this lesson you will need **OpenRefine** (formerly Google Refine) and a
web browser. Basic installation steps are provided on this page.
The OpenRefine [installation manual](https://openrefine.org/docs/manual/installing)
provides more details about installation, upgrades and configuration.
Note: this is a Java program that runs on your machine (not in the cloud).
It runs inside your browser, but no web connection is needed for this lesson.
:::::::::::::::::::::::::::::::::::::: callout
### Administrator rights
You do not need administrative rights on the computer to *install* OpenRefine.
However, if anti-malware software blocks OpenRefine when you try to start it,
you may need administrative rights to allow OpenRefine to *run*.
OpenRefine is safe to run.
:::::::::::::::::::::::::::::::::::::::::::::::
::::::::::::::::::::::::::::::::::::::::::::::::::This is also why the styling for the callout block looks like a prereq block.
zkamvar
commented
1 year ago Also, for future, you can give me your session information using the built in sessionInfo() function or the {sessioninfo} package for a more detailed view:
sessionInfo()sessioninfo::session_info()bencomp
commented
1 year ago Thanks for these function pointers and comments! Perhaps my versions are a tiny bit behind yours (downloaded from the Netherlands mirror, which could be behind?), although I also have packages that you don't.
Looking at https://github.com/carpentries/sandpaper/pull/467/files#diff-b6d0e1001a0e8491b46773f0256ae272453ba38c9f2e666d4f7ce49197c8068dR11-R16 (from #467) I see that this div also has the class callout-title that I think causes the issue. Anyway, you are correct that the issue does not occur on the live website.
I will see if it continues to occur.
zkamvar
commented
1 year ago I think the only way you would have seen this with version 0.12.0 is if you had the development version installed, but even then, I think I only switched it to version 0.12.0 after I fixed the callout blocks anchor IDs (which was an impetus for me to bump the minor version), so it's a bit of a mystery. A slow mirror wouldn't affect that because {sandpaper} only comes from The Carpentries R Universe, not CRAN.
That being said, considering it's no longer a problem on the deployed site, I will close this for now. Please reopen if this is still happening.
zkamvar
commented
1 year ago I'm reopening this because I've discovered that the the source of this issue is due to a change in behaviour between pandoc 2 and 3 in https://github.com/carpentries/workbench/issues/59#issuecomment-1645623167:
lsn <- usethis::create_from_github("zkamvar/test-workbench-59", destdir = tempdir())
#> ℹ Defaulting to 'https' Git protocol
#> ✔ Setting `fork = FALSE`
#> ✔ Creating '/tmp/Rtmp24N1j4/test-workbench-59/'
#> ✔ Cloning repo from 'https://github.com/zkamvar/test-workbench-59.git' into '/tmp/Rtmp24N1j4/test-workbench-59'
#> ✔ Setting active project to '/tmp/Rtmp24N1j4/test-workbench-59'
#> ℹ Default branch is 'main'
#> ✔ Setting active project to '<no active project>'
pandoc::pandoc_activate("3.1.3")
#> ✔ Version '3.1.3' is now the active one.
#> ℹ Pandoc version also activated for rmarkdown functions.
sandpaper::build_lesson(lsn, quiet = TRUE, preview = FALSE)
idx <- xml2::read_html(fs::path(lsn, "site/docs/index.html"))
xml2::xml_find_first(idx, ".//div[starts-with(@class, 'callout')]") |> as.character() |> writeLines()
#> <div id="callout1" class="callout">
#> <div class="callout-square">
#> <i class="callout-icon" data-feather="bell"></i>
#> </div>
#> <div class="section level3 callout-title callout-inner">
#> <h3 class="callout-title" id="being-certain-which-system-your-terminal-is-connected-to">Being certain which system your terminal is
#> connected to<a class="anchor" aria-label="anchor" href="#being-certain-which-system-your-terminal-is-connected-to"></a>
#> </h3>
#> <div class="callout-content">
#> <p>If you ever need to be certain which system a terminal you are using
#> is connected to then use the following command:
#> <code>$ hostname</code>.</p>
#> </div>
#> </div>
#> </div>Created on 2023-07-21 with reprex v2.0.2
I believe it's coming from this part of the lua filter where we place the callout header (which has the "callout-title" class) inside of the "callout-inner" block:
zkamvar
commented
1 year ago A bit more background for clarification: pandoc operates in this way:
flowchart TB
input -->|"reader (+[options])"| AST
AST -->|filter| AST
AST -->|"writer (--[options])"| outputThe reason why we are seeing the difference in behaviour is because the HTML-writer-specific option --section-divs is applied after the filter transformation. Thus, when pandoc sees the section with a heading that contains a class that is not present in the section class, it the --section-divs in pandoc version 3 will automatically inherit it, causing the CSS rule to be applied to the entire div and not just the header.
The thing is, we already correct for this here and I think the issue is that we are searching for "callout ", when pandoc 3 collapses duplicate classes, so I think the solution is to search that the div starts with callout
bencomp
commented
1 year ago You're probably on to something, @zkamvar! A change in Pandoc across major versions would explain such changes in output.
If I read it correctly, --section-divs 'moves' identifiers from the header to the enclosing section, but would it also move or copy classes?
Would a workaround be to adjust the CSS selector from .callout-title to h3.callout-title? Then only the header should be text-transformed.
zkamvar
commented
1 year ago Would a workaround be to adjust the CSS selector from .callout-title to h3.callout-title? Then only the header should be text-transformed.
This indeed would work and it would be a good idea to implement that.
It's important to note that only "callout" blocks (but not "discussion", "keypoints" or any other blocks) are affected because pandoc 3 (rightfully) deduplicates classes, thus my selection of starts-with(@class, 'callout ') does not match <div class='callout'>. in the post-processing step.
If I read it correctly, --section-divs 'moves' identifiers from the header to the enclosing section, but would it also move or copy classes?
Yes. It's been a topic of some debate in pandoc for the past few years: https://github.com/jgm/pandoc/issues/5965, which is probably why I've never gone for the h3.callout-title selector, because I never knew where it was going to end up.
tbooth
commented
8 months ago Hi,
Just wanted to chip in and say I'm seeing this bug when building lessons (I'm currently converting https://github.com/carpentries-incubator/snakemake-novice-bioinformatics from the old format). I've installed all the Sandpaper dependencies, including Pandoc and R, in a fresh Conda environment on Ubuntu.
In my case, every single callout has this problem where callout-title is incorrectly applied to the whole thing.
The quick fix was to force a downgrade of Pandoc in the conda env:
$ conda install pandoc=2.19.2It might be worth noting this on the requirements document (https://carpentries.github.io/sandpaper-docs/#required) as Pandoc 2.x does seem to be the preferred version just now and is explicitly used in the GitHub actions.
froggleston
commented
8 months ago Apologies as I'm coming fairly new into the Workbench and its maintenance, and my R is very much lapsed!
From my reading through and checking the instructor-training lesson as per Nathaniel's report, the only callouts affected by this are the callout callouts, and not challenge, etc. In the latter, the resulting div has two classes, callout and the name of the block, e.g. using 04-expertise.md as the test case:
<div id="what-is-an-expert" class="callout discussion">
In the former, the resulting div has only one class, callout:
<div id="callout1" class="callout">
This seems to be the reason that the existing fix_callouts() function is not gathering up these callout-only divs.
Is there a reason why we couldn't use an additional XPath selector as:
callouts <- xml2::xml_find_all(idx, ".//div[@class='callout']")
In addition to using the current starts-with? https://github.com/carpentries/sandpaper/blob/eb009faca249bac5b999ac5449990329d3eb7d0d/R/utils-xml.R#L200C41-L200C83
Using the specific class selector seems to grab the correct callout parent divs, and would require no other changes to other code as far as I can tell? I've tested this with pandoc 2.19.2 and 3.11.1 and both seem to produce the same "clean" callout-inner class structure despite the pandoc3 behaviour.
froggleston
commented
8 months ago After a bit more testing this morning, it seems:
callouts <- xml2::xml_find_all(idx, ".//div[starts-with(@class, 'callout ')] | .//div[@class='callout']")
Produces consistent results across pandoc 2.x and 3.x. I'll raise a PR.
froggleston
commented
8 months ago This should now be resolved in #574 so please do test and let us know of any other issues.
ErinBecker
commented
5 months ago Can this issue be closed @froggleston?
I don't know why it happens, but sometimes the CSS class
callout-titleis added to the<h3>element (correctly) and its parent<div>element (incorrectly). When that happens, the body of the callout inherits the CSS directives for the title:(This is from my local build; I see no difference before and after updating to the latest sandpaper.)
I also don't see an immediate pattern – it doesn't happen with every callout or only within nested callouts.
The files that show this error are in https://github.com/datacarpentry/openrefine-socialsci/pull/154, if anyone would like to try to reproduce this issue.