Merge pull request #474 from carpentries/fix-extra-page-sidebar

fix extra page sidebar

DESCRIPTION

@@ -1,6 +1,6 @@
 Package: sandpaper
 Title: Create and Curate Carpentries Lessons
-Version: 0.12.1
+Version: 0.12.2
 Authors@R: c(
     person(given = "Zhian N.",
            family = "Kamvar",

NEWS.md

@@ -1,4 +1,11 @@
-# sandpaper 0.12.1 (unreleased)
+# sandpaper 0.12.2 (2023-05-29)
+
+## BUG FIX
+
+* A bug where the sidebar for non-episode pages had extra commas was fixed
+  (reported: @zkamvar, #473; fixed: @zkamvar, #474)
+
+# sandpaper 0.12.1 (2023-05-26)
 
 ## BUG FIX
 

R/utils-sidebar.R

@@ -141,6 +141,7 @@ update_sidebar <- function(sidebar = NULL, nodes = NULL, path_md = NULL, title =
       item <- grep(paste0("[<]a href=['\"]", this_page, "['\"]"),
         this_sidebar)
       if (length(item) == 0) {
+        sidebar$set("sidebar", paste(this_sidebar, collapse = "\n"))
         return(sidebar)
       }
       # The title should stay the same.

R/utils-varnish.R

@@ -31,8 +31,8 @@ varnish_vars <- function() {
   )
 }
 
-#' Set the necessary common global variables for use in the {varnish} template. 
-#' 
+#' Set the necessary common global variables for use in the {varnish} template.
+#'
 #' This will enforce four global lists:
 #'
 #'  1. `.resources`, which is equivalent to the output of `get_source_list()`
@@ -45,7 +45,7 @@ varnish_vars <- function() {
 #'  - `sidebar` This is generated from [create_sidebar()] and is the same in the
 #'    learner and instructor globals with the exception of the first element.
 #'  - `more` This is the "More" dorpdown menu, which is created via [create_resources_dropdown()].
-#'  - `resources` The same as "More", but positioned on the mobile sidebar. 
+#'  - `resources` The same as "More", but positioned on the mobile sidebar.
 #'  - `{sandpaper,varnish,pegboard}_version` package versions of each package.
 #'
 #' @param path the path to the lesson
@@ -65,7 +65,7 @@ set_globals <- function(path) {
   idx <- these_resources[["."]]
   idx <- idx[as_html(idx) == "index.html"]
   instructor_sidebar <- create_sidebar(c(idx, these_resources[["episodes"]]))
-  # check if we have a title in the index sidebar and replace with 
+  # check if we have a title in the index sidebar and replace with
   # "summary and schedule" if it does not exist.
   idx_item <- xml2::read_html(instructor_sidebar[[1]])
   idx_link <- xml2::xml_find_first(idx_item, ".//a")
@@ -81,13 +81,13 @@ set_globals <- function(path) {
   learner_sidebar[[1]] <- sub("Schedule", "Setup", sindex)
 
   # Resources
-  learner <- create_resources_dropdown(these_resources[["learners"]], 
+  learner <- create_resources_dropdown(these_resources[["learners"]],
     "learners")
-  instructor <- create_resources_dropdown(these_resources[["instructors"]], 
+  instructor <- create_resources_dropdown(these_resources[["instructors"]],
     "instructors")
   pkg_versions <- varnish_vars()
 
-  learner_globals$set(key = NULL, 
+  learner_globals$set(key = NULL,
     c(list(
       aio = TRUE,
       instructor = FALSE,
@@ -96,7 +96,7 @@ set_globals <- function(path) {
       resources = paste(learner$resources, collapse = "")
     ), pkg_versions)
   )
-  instructor_globals$set(key = NULL, 
+  instructor_globals$set(key = NULL,
     c(list(
       aio = TRUE,
       instructor = TRUE,

tests/testthat/_snaps/utils-sidebar.md

@@ -1,7 +1,7 @@
-# a sidebar can be and will have sequential numbers
+# a sidebar can be created with a specific episode and will have sequential numbers
 
     Code
-      writeLines(create_sidebar(chapters, name = "two.md", html = html))
+      writeLines(sb)
     Output
       <div class="accordion accordion-flush" id="accordionFlush1">
         <div class="accordion-item">

tests/testthat/test-utils-sidebar.R

@@ -23,8 +23,12 @@ test_that("sidebar headings can contain html within", {
 })
 
 
-test_that("a sidebar can be and will have sequential numbers", {
+test_that("a sidebar can be created with a specific episode and will have sequential numbers", {
 
+  # NOTE: 2023-05-29 I believe this test is sort of defunct because we are
+  # testing here how to create a sidebar given a specific episode, but we no
+  # longer use this pattern in the lesson, so this test and the `name` option
+  # for `create_sidebar()` should be removed.
   mockr::local_mock(get_navbar_info = function(i) {
     list(pagetitle = toupper(i), text = paste("text", i), href = as_html(i))
   })
@@ -37,7 +41,50 @@ test_that("a sidebar can be and will have sequential numbers", {
   <p>This is how you build your plots iteratively</p>
   </section>"
   chapters <- c("index.md", "one.md", "two.md", "three.md")
-  expect_snapshot(writeLines(create_sidebar(chapters, name = "two.md", html = html)))
+  sb <- create_sidebar(chapters, name = "two.md", html = html)
+  expect_snapshot(writeLines(sb))
+
+})
+
+
+test_that("updating a sidebar for all pages modifies appropriately", {
+
+  mockr::local_mock(get_navbar_info = function(i) {
+    list(pagetitle = toupper(i), text = paste("text", i), href = as_html(i))
+  })
+  html <- "<section id='plotting'>
+  <h2 class='section-heading'>Plotting with <strong><code>ggplot2</code></strong>
+  <p>This is how you plot with <code>ggplot2</code></p>
+  </section>
+  <section id='building'>
+  <h2 class='section-heading'>Building your plots iteratively</h2>
+  <p>This is how you build your plots iteratively</p>
+  </section>"
+  chapters <- c("index.md", "one.md", "two.md", "three.md")
+  sb <- create_sidebar(chapters, html = html)
+  extra_store <- .list_store()
+  extra_store$update(c(list(sidebar = sb), get_navbar_info("images.md")))
+  ep_store <- extra_store$copy()
+
+  xhtml <- xml2::read_html(html)
+
+  # sidebar update of _extra_ content will _not_ update the sidebar -----------
+  update_sidebar(extra_store, xhtml, "images.md")
+  expect_length(extra_store$get()[["sidebar"]], 1L)
+  expect_identical(extra_store$get()[["sidebar"]], paste(sb, collapse = "\n"))
+  extra_nodes <- xml2::read_html(extra_store$get()[["sidebar"]])
+  extra_current <- xml2::xml_find_all(extra_nodes, ".//span[@class='current-chapter']")
+  expect_length(extra_current, 0L)
+
+  # sidebar update of episode content will update the sidebar -----------------
+  ep_store$update(get_navbar_info("two.md"))
+  update_sidebar(ep_store, xhtml, "two.md")
+  expect_length(ep_store$get()[["sidebar"]], 1L)
+  expect_false(identical(ep_store$get()[["sidebar"]], paste(sb, collapse = "\n")))
+  ep_nodes <- xml2::read_html(ep_store$get()[["sidebar"]])
+  ep_current <- xml2::xml_find_all(ep_nodes, ".//span[@class='current-chapter']")
+  expect_length(ep_current, 1L)
+
 })