Merge pull request #472 from carpentries/current-page-number-432

keep current episode number in sidebar

NEWS.md

@@ -1,10 +1,18 @@
 # sandpaper 0.12.1 (unreleased)
 
-MISC
-----
+## BUG FIX
+
+* The current page of the sidebar no longer hides the episode number. 
+  (reported: @cynthiaftw, https://github.com/carpentries/workbench/issues/42 and
+  #432; fixed: @zkamvar, #472)
+- metadata for episodes with titles containing markup no longer include that
+  markup in the metadata (@zkamvar, #472)
+
+## MISC
 
 * The internal function `sandpaper:::check_pandoc()` now points to the correct
-  URL to download RStudio, which moved after the migration to posit. 
+  URL to download RStudio, which moved after the migration to posit (@zkamvar,
+  #471) 
 
 # sandpaper 0.12.0 (2023-05-19)
 

R/utils-metadata.R

@@ -13,6 +13,10 @@ fill_metadata_template <- function(meta) {
   if (endsWith(local_meta$url, "/")) {
     local_meta$url <- paste0(local_meta$url, "index.html")
   }
+  title <- local_meta$pagetitle
+  if (grepl("<", title, fixed = TRUE)) {
+    local_meta$pagetitle <- xml2::xml_text(xml2::read_html(title))
+  }
   json <- local_meta[["metadata_template"]]
   json <- whisker::whisker.render(json, local_meta)
   json

R/utils-sidebar.R

@@ -129,21 +129,34 @@ create_sidebar <- function(chapters, name = "", html = "<a href='https://carpent
   res
 }
 
-update_sidebar <- function(sidebar = NULL, nodes = NULL, path_md = NULL, title = NULL, instructor = TRUE) {
+update_sidebar <- function(sidebar = NULL, nodes = NULL, path_md = NULL, title = NULL, instructor = TRUE, item = NULL) {
   if (is.null(sidebar)) return(sidebar)
+  this_page <- as_html(path_md)
   # NOTE: this is the place we need to modify to address
   # https://github.com/carpentries/workbench/issues/42
   if (inherits(sidebar, "list-store")) {
+    this_sidebar <- sidebar$get()[["sidebar"]]
     # if it's a list store, then we need to get the sidebar and update itself
-    title <- if (is.null(title)) sidebar$get()[["pagetitle"]] else title
-    sb <- update_sidebar(sidebar$get()[["sidebar"]], nodes, path_md, title,
-      instructor)
+    if (is.null(title)) {
+      item <- grep(paste0("[<]a href=['\"]", this_page, "['\"]"),
+        this_sidebar)
+      if (length(item) == 0) {
+        return(sidebar)
+      }
+      # The title should stay the same.
+      side_nodes <- xml2::xml_find_first(xml2::read_xml(this_sidebar[item]),
+        ".//a")
+      title <- paste(as.character(xml2::xml_contents(side_nodes)), collapse = "")
+    }
+    sb <- update_sidebar(this_sidebar, nodes, path_md, title, instructor,
+      item = item)
     sidebar$set("sidebar", paste(sb, collapse = "\n"))
   }
-  this_page <- as_html(path_md)
-  to_change <- grep(paste0("[<]a href=['\"]", this_page, "['\"]"), sidebar)
-  if (length(to_change)) {
-    sidebar[to_change] <- create_sidebar_item(nodes, title, "current")
+  if (is.null(item)) {
+    item <- grep(paste0("[<]a href=['\"]", this_page, "['\"]"), sidebar)
+  }
+  if (length(item)) {
+    sidebar[item] <- create_sidebar_item(nodes, title, "current")
   }
   sidebar
 }

tests/testthat/_snaps/build_lesson.md

@@ -6,7 +6,7 @@
       <a href="../key-points.html">Learner View</a>
       <a href="index.html">Summary and Schedule</a>
       <a href="introduction.html">1. introduction</a>
-      <a href="second-episode.html">2. Second Episode!</a>
+      <a href="second-episode.html">2. <em>Second</em> Episode!</a>
       <a href="../instructor/key-points.html">Key Points</a>
       <a href="../instructor/instructor-notes.html">Instructor Notes</a>
       <a href="../instructor/images.html">Extract All Images</a>
@@ -20,7 +20,7 @@
       <a href="instructor/key-points.html">Instructor View</a>
       <a href="index.html">Summary and Setup</a>
       <a href="introduction.html">1. introduction</a>
-      <a href="second-episode.html">2. Second Episode!</a>
+      <a href="second-episode.html">2. <em>Second</em> Episode!</a>
       <a href="key-points.html">Key Points</a>
       <a href="reference.html#glossary">Glossary</a>
       <a href="profiles.html">Learner Profiles</a>

tests/testthat/test-build_lesson.R

@@ -197,7 +197,7 @@ test_that("Lesson websites contains instructor metadata", {
 
 test_that("single files can be built", {
 
-  create_episode("Second Episode!", path = tmp)
+  create_episode("_Second_ Episode!", path = tmp)
   suppressMessages(s <- get_episodes(tmp))
   set_episodes(tmp, s, write = TRUE)
 
@@ -284,6 +284,21 @@ test_that("HTML files are present and have the correct elements", {
   )))
 })
 
+
+test_that("Active episode contains sidebar number", {
+  ep <- readLines(fs::path(sitepath, "second-episode.html"))
+  xml <- xml2::read_html(paste(ep, collapse = ""))
+
+  # Instructor sidebar is formatted properly
+  sidebar <- xml2::xml_find_all(xml, ".//div[@class='sidebar']")
+  expect_length(sidebar, 1L)
+  this_ep <- xml2::xml_find_first(sidebar, ".//span[@class='current-chapter']")
+  this_title <- as.character(xml2::xml_contents(this_ep))
+  this_title <- trimws(paste(this_title, collapse = ""))
+  expect_equal(this_title, "2. <em>Second</em> Episode!")
+})
+
+
 test_that("files will not be rebuilt unless they change in content", {
 
   skip_if_not(rmarkdown::pandoc_available("2.11"))

tests/testthat/test-manage_deps.R

@@ -59,8 +59,7 @@ test_that("manage_deps() will create a renv folder", {
   # NOTE: these tests are still not very specific here...
   suppressMessages({
     build_markdown(lsn, quiet = FALSE) %>%
-      expect_message("Consent to use package cache provided") %>%
-      expect_output("knitr")
+      expect_message("Consent to use package cache provided")
   })
 
   expect_true(fs::dir_exists(rnv))

tests/testthat/test-utils-sidebar.R

@@ -15,7 +15,7 @@ test_that("sidebar headings can contain html within", {
   # The result is a list element with two items
   expect_length(li, 2)
   # one heading has a child node within
-  expect_equal(xml2::xml_length(xml2::xml_children(li)), 
+  expect_equal(xml2::xml_length(xml2::xml_children(li)),
     c(1, 0))
   # the anchors are the URIs
   expect_equal(xml2::xml_text(xml2::xml_find_all(li, ".//@href")),
@@ -24,7 +24,7 @@ test_that("sidebar headings can contain html within", {
 
 
 test_that("a sidebar can be and will have sequential numbers", {
-  
+
   mockr::local_mock(get_navbar_info = function(i) {
     list(pagetitle = toupper(i), text = paste("text", i), href = as_html(i))
   })