From f6b97868bae2de641833fc07852117cd5978f9f4 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 10:07:31 -0700 Subject: [PATCH 01/10] add build_images function --- R/build_all_images.R | 42 ++++++++++++++++++++++++++++++++++++++++++ R/build_site.R | 2 ++ R/utils-aggregate.R | 1 + 3 files changed, 45 insertions(+) create mode 100644 R/build_all_images.R diff --git a/R/build_all_images.R b/R/build_all_images.R new file mode 100644 index 000000000..891615506 --- /dev/null +++ b/R/build_all_images.R @@ -0,0 +1,42 @@ +#' @rdname build_agg +build_images <- function(pkg, pages = NULL, quiet = FALSE) { + build_agg_page(pkg = pkg, + pages = pages, + title = "All Images", + slug = "images", + aggregate = "/figure", + prefix = FALSE, + quiet = quiet) +} + +make_images_section <- function(name, contents, parent) { + title <- names(name) + uri <- sub("^images-", "", name) + new_section <- "
+

{title}

+
+
" + section <- xml2::read_xml(glue::glue(new_section)) + + for (element in seq_along(contents)) { + content <- contents[[element]] + alt <- xml2::xml_text(xml2::xml_find_all(content, "./img/@alt")) + n <- length(alt) + xml2::xml_add_child(section, "h3", glue::glue("Figure {element}"), + id = glue::glue("{name}-figure-{element}")) + for (i in seq_along(alt)) { + txt <- alt[[i]] + if (length(txt) == 0) { + txt <- "[no alt-text]" + } + if (txt == "") { + txt <- "[decorative]" + } + desc <- glue::glue("Image {i} of {n}: {sQuote(txt)}") + xml2::xml_add_child(section, "p", 'aria-hidden'="true", desc) + } + xml2::xml_add_child(section, contents[[element]]) + xml2::xml_add_child(section, "hr") + } + xml2::xml_add_child(parent, section) +} diff --git a/R/build_site.R b/R/build_site.R index 5e968f1b4..ca6c2f7bb 100644 --- a/R/build_site.R +++ b/R/build_site.R @@ -103,6 +103,8 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr build_keypoints(pkg, pages = html_pages, quiet = quiet) if (!quiet) cli::cli_rule(cli::style_bold("Creating All-in-one page")) build_aio(pkg, pages = html_pages, quiet = quiet) + if (!quiet) cli::cli_rule(cli::style_bold("Creating Images page")) + build_images(pkg, pages = html_pages, quiet = quiet) build_sitemap(pkg$dst_path, paths = html_pages$paths, quiet = quiet) diff --git a/R/utils-aggregate.R b/R/utils-aggregate.R index 530db7989..9bdffff02 100644 --- a/R/utils-aggregate.R +++ b/R/utils-aggregate.R @@ -12,6 +12,7 @@ #' - `$instructor`: all of the pages in the instructor view #' - `$paths`: the absolute paths for the pages #' +#' @keywords internal #' @examples #' tmpdir <- tempfile() #' on.exit(fs::dir_delete(tmpdir)) From 52f5cd4cee4129e876acaf2ffff2ebd66e35b6d5 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 10:13:36 -0700 Subject: [PATCH 02/10] fix docs --- man/build_agg.Rd | 7 +++++-- man/provision.Rd | 2 +- man/read_all_html.Rd | 1 + 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/man/build_agg.Rd b/man/build_agg.Rd index 9e3ecb38c..287c4f2db 100644 --- a/man/build_agg.Rd +++ b/man/build_agg.Rd @@ -1,14 +1,17 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/build_aio.R, R/build_keypoints.R, -% R/utils-aggregate.R +% Please edit documentation in R/build_aio.R, R/build_all_images.R, +% R/build_keypoints.R, R/utils-aggregate.R \name{build_aio} \alias{build_aio} +\alias{build_images} \alias{build_keypoints} \alias{build_agg_page} \title{Build a page for aggregating common elements} \usage{ build_aio(pkg, pages = NULL, quiet = FALSE) +build_images(pkg, pages = NULL, quiet = FALSE) + build_keypoints(pkg, pages = NULL, quiet = FALSE) build_agg_page( diff --git a/man/provision.Rd b/man/provision.Rd index da2dcf56d..ffaf68ddb 100644 --- a/man/provision.Rd +++ b/man/provision.Rd @@ -3,7 +3,7 @@ \name{provision_agg_page} \alias{provision_agg_page} \alias{provision_extra_template} -\title{Provision an "extra" page in a lesson} +\title{Provision an aggregate page in a lesson} \usage{ provision_agg_page(pkg, title = "Key Points", slug = "key-points", quiet) diff --git a/man/read_all_html.Rd b/man/read_all_html.Rd index dfb53c70c..6271a3570 100644 --- a/man/read_all_html.Rd +++ b/man/read_all_html.Rd @@ -31,3 +31,4 @@ writeLines("

Learner

", fs::path(tmpdir, "index.html")) sandpaper:::read_all_html(tmpdir) } +\keyword{internal} From f6e0ea96c1cdfc3892123621c884ab939aa71f13 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 10:52:51 -0700 Subject: [PATCH 03/10] rename file --- R/{build_all_images.R => build_images.R} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename R/{build_all_images.R => build_images.R} (100%) diff --git a/R/build_all_images.R b/R/build_images.R similarity index 100% rename from R/build_all_images.R rename to R/build_images.R From 9700ee047cd4ea7eba180d86bcf8e9b8186aea26 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 11:04:42 -0700 Subject: [PATCH 04/10] document images --- R/build_images.R | 22 ++++++++++++++++++++++ _pkgdown.yml | 1 + man/build_agg.Rd | 2 +- man/make_images_section.Rd | 37 +++++++++++++++++++++++++++++++++++++ 4 files changed, 61 insertions(+), 1 deletion(-) create mode 100644 man/make_images_section.Rd diff --git a/R/build_images.R b/R/build_images.R index 891615506..b2ee77b01 100644 --- a/R/build_images.R +++ b/R/build_images.R @@ -9,6 +9,28 @@ build_images <- function(pkg, pages = NULL, quiet = FALSE) { quiet = quiet) } +#' Make a section of aggregated images +#' +#' This will insert xml figure nodes into the images page, printing the alt text +#' descriptions for users who are not using screen readers. +#' +#' @param name the name of the section, (may or may not be prefixed with `images-`) +#' @param contents an `xml_nodeset` of figure elements from [get_content()] +#' @param parent the parent div of the images page +#' @return the section that was added to the parent +#' +#' @keywords internal +#' @seealso [build_images()], [get_content()] +#' @examples +#' if (FALSE) { +#' lsn <- "/path/to/lesson" +#' pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) +#' +#' # read in the All in One page and extract its content +#' img <- get_content("images", content = "self::*", pkg = pkg) +#' fig_content <- get_content("01-introduction", content = "/figure", pkg = pkg) +#' make_images_section("01-introduction", contents = fig_content, parent = img) +#' } make_images_section <- function(name, contents, parent) { title <- names(name) uri <- sub("^images-", "", name) diff --git a/_pkgdown.yml b/_pkgdown.yml index 44fd819f0..ea8372a83 100644 --- a/_pkgdown.yml +++ b/_pkgdown.yml @@ -90,6 +90,7 @@ reference: - provision_extra_template - get_content - make_aio_section + - make_images_section - build_agg_page - title: "[Internal] Resource Discovery/Management" desc: > diff --git a/man/build_agg.Rd b/man/build_agg.Rd index 287c4f2db..a0cc10f31 100644 --- a/man/build_agg.Rd +++ b/man/build_agg.Rd @@ -1,5 +1,5 @@ % Generated by roxygen2: do not edit by hand -% Please edit documentation in R/build_aio.R, R/build_all_images.R, +% Please edit documentation in R/build_aio.R, R/build_images.R, % R/build_keypoints.R, R/utils-aggregate.R \name{build_aio} \alias{build_aio} diff --git a/man/make_images_section.Rd b/man/make_images_section.Rd new file mode 100644 index 000000000..e213b6e00 --- /dev/null +++ b/man/make_images_section.Rd @@ -0,0 +1,37 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/build_images.R +\name{make_images_section} +\alias{make_images_section} +\title{Make a section of aggregated images} +\usage{ +make_images_section(name, contents, parent) +} +\arguments{ +\item{name}{the name of the section, (may or may not be prefixed with \verb{images-})} + +\item{contents}{an \code{xml_nodeset} of figure elements from \code{\link[=get_content]{get_content()}}} + +\item{parent}{the parent div of the images page} +} +\value{ +the section that was added to the parent +} +\description{ +This will insert xml figure nodes into the images page, printing the alt text +descriptions for users who are not using screen readers. +} +\examples{ +if (FALSE) { +lsn <- "/path/to/lesson" +pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) + +# read in the All in One page and extract its content +img <- get_content("images", content = "self::*", pkg = pkg) +fig_content <- get_content("01-introduction", content = "/figure", pkg = pkg) +make_images_section("01-introduction", contents = fig_content, parent = img) +} +} +\seealso{ +\code{\link[=build_images]{build_images()}}, \code{\link[=get_content]{get_content()}} +} +\keyword{internal} From 63e41cec07ea24f57fb9ef519d0895f36f904adb Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 16:31:40 -0700 Subject: [PATCH 05/10] simplify build_agg function --- R/utils-aggregate.R | 57 +++++++++++++++++++++++++++------------------ man/build_agg.Rd | 9 +++++-- man/get_content.Rd | 5 +++- man/provision.Rd | 7 +++--- 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/R/utils-aggregate.R b/R/utils-aggregate.R index 9bdffff02..01155be88 100644 --- a/R/utils-aggregate.R +++ b/R/utils-aggregate.R @@ -44,8 +44,9 @@ read_all_html <- function(path) { #' @param pkg an object created via [pkgdown::as_pkgdown()] of a lesson. #' @param title the new page title #' @param slug the slug for the page (e.g. "aio" will become "aio.html") -#' @param quiet if `TRUE`, no messages will be emitted. If FALSE, -#' pkgdown will report creation of the temporary file. +#' @param new if `TRUE`, (default), the page will be generated +#' from a new template. If `FALSE`, the page is assumed to have been pre-built and +#' should be appended to. #' @return #' - `provision_agg_page()`: a list: #' - `$learner`: an `xml_document` templated for the learner page @@ -93,21 +94,26 @@ read_all_html <- function(path) { #' provision_agg_page(pkg, title = "All In One", slug = "aio", quiet = FALSE) #' #' } -provision_agg_page <- function(pkg, title = "Key Points", slug = "key-points", quiet) { - if (is.null(.html$get()$template$extra)) { - provision_extra_template(pkg) +provision_agg_page <- function(pkg, title = "Key Points", slug = "key-points", new = FALSE) { + if (new) { + if (is.null(.html$get()$template$extra)) { + provision_extra_template(pkg) + } + learner <- .html$get()$template$extra$learner + instructor <- .html$get()$template$extra$instructor + learner <- gsub("--FIXME TITLE", title, learner) + instructor <- gsub("--FIXME TITLE", title, instructor) + learner <- gsub("--FIXME", slug, learner) + instructor <- gsub("--FIXME", slug, instructor) + } else { + uri <- as_html(slug) + learner <- fs::path(pkg$dst_path, uri) + instructor <- fs::path(pkg$dst_path, "instructor", uri) } - learner <- .html$get()$template$extra$learner - instructor <- .html$get()$template$extra$instructor - learner <- gsub("--FIXME TITLE", title, learner) - instructor <- gsub("--FIXME TITLE", title, instructor) - learner <- gsub("--FIXME", slug, learner) - instructor <- gsub("--FIXME", slug, instructor) - return(list(learner = xml2::read_html(learner), instructor = xml2::read_html(instructor), - needs_episodes = TRUE) + needs_episodes = new) ) } @@ -157,8 +163,13 @@ section_fun <- function(slug) { #' @param aggregate a selector for the lesson content you want to aggregate. #' The default is "section", which will aggregate all sections, but nothing #' outside of the sections. To grab everything in the page, use "*" +#' @param append a selector for the section of the page where the aggregate data +#' should be placed. This defaults to "self::*", which indicates that the +#' entire page should be appended. #' @param prefix flag to add a prefix for the aggregated sections. Defaults to #' `FALSE`. +#' @param quiet if `TRUE`, no messages will be emitted. If FALSE, pkgdown will +#' report creation of the temporary file. #' @return NULL, invisibly. This is called for its side-effect #' #' @details @@ -191,11 +202,12 @@ section_fun <- function(slug) { #' build_aio(pkg, htmls, quiet = FALSE) #' build_keypoints(pkg, htmls, quiet = FALSE) #' } -build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "section", prefix = FALSE, quiet = FALSE) { +build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "section", append = "self::*", prefix = FALSE, quiet = FALSE) { path <- root_path(pkg$src_path) out_path <- pkg$dst_path this_lesson(path) - agg <- provision_agg_page(pkg, title = title, slug = slug, quiet) + new_content <- append == "self::*" + agg <- provision_agg_page(pkg, title = title, slug = slug, new = new_content) if (agg$needs_episodes) { remove_fix_node(agg$learner, slug) remove_fix_node(agg$instructor, slug) @@ -203,16 +215,12 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s make_section <- section_fun(slug) - learn <- get_content(agg$learner, content = "section", label = TRUE) - learn_parent <- get_content(agg$learner, content = "self::*") - - instruct <- get_content(agg$instructor, content = "section", label = TRUE) - instruct_parent <- get_content(agg$instructor, content = "self::*") + learn_parent <- get_content(agg$learner, content = append) + instruct_parent <- get_content(agg$instructor, content = append) the_episodes <- .resources$get()[["episodes"]] the_slugs <- get_slug(the_episodes) the_slugs <- if (prefix) paste0(slug, "-", the_slugs) else the_slugs - old_names <- names(learn) for (episode in seq(the_episodes)) { ep_learn <- ep_instruct <- the_episodes[episode] @@ -224,7 +232,7 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s } ep_title <- as.character(xml2::xml_contents(get_content(ep_learn, ".//h1"))) names(ename) <- paste(ep_title, collapse = "") - ep_learn <- get_content(ep_learn, content = aggregate, pkg = pkg) + ep_learn <- get_content(ep_learn, content = aggregate, pkg = pkg) ep_instruct <- get_content(ep_instruct, content = aggregate, pkg = pkg, instructor = TRUE) make_section(ename, ep_learn, learn_parent) make_section(ename, ep_instruct, instruct_parent) @@ -273,11 +281,14 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s #' lsn <- "/path/to/lesson" #' pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) #' -#' # for AiO pages, this will return only sections: +#' # for AiO pages, this will return only the top-level sections: #' get_content("aio", content = "section", label = TRUE, pkg = pkg) #' #' # for episode pages, this will return everything that's not template #' get_content("01-introduction", pkg = pkg) +#' +#' # for things that are within lessons but we don't know their exact location, +#' # we can prefix a `/` to double up the slash, which will produce #' #' } get_content <- function(episode, content = "*", label = FALSE, pkg = NULL, diff --git a/man/build_agg.Rd b/man/build_agg.Rd index a0cc10f31..1d3137ca5 100644 --- a/man/build_agg.Rd +++ b/man/build_agg.Rd @@ -20,6 +20,7 @@ build_agg_page( title = NULL, slug = NULL, aggregate = "section", + append = "self::*", prefix = FALSE, quiet = FALSE ) @@ -30,8 +31,8 @@ build_agg_page( \item{pages}{output from the function \code{\link[=read_all_html]{read_all_html()}}: a nested list of \code{xml_document} objects representing episodes in the lesson} -\item{quiet}{if \code{TRUE}, no messages will be emitted. If FALSE, -pkgdown will report creation of the temporary file.} +\item{quiet}{if \code{TRUE}, no messages will be emitted. If FALSE, pkgdown will +report creation of the temporary file.} \item{title}{the new page title} @@ -41,6 +42,10 @@ pkgdown will report creation of the temporary file.} The default is "section", which will aggregate all sections, but nothing outside of the sections. To grab everything in the page, use "*"} +\item{append}{a selector for the section of the page where the aggregate data +should be placed. This defaults to "self::*", which indicates that the +entire page should be appended.} + \item{prefix}{flag to add a prefix for the aggregated sections. Defaults to \code{FALSE}.} } diff --git a/man/get_content.Rd b/man/get_content.Rd index 4cc28ae96..d19896b8f 100644 --- a/man/get_content.Rd +++ b/man/get_content.Rd @@ -46,12 +46,15 @@ if (FALSE) { lsn <- "/path/to/lesson" pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) -# for AiO pages, this will return only sections: +# for AiO pages, this will return only the top-level sections: get_content("aio", content = "section", label = TRUE, pkg = pkg) # for episode pages, this will return everything that's not template get_content("01-introduction", pkg = pkg) +# for things that are within lessons but we don't know their exact location, +# we can prefix a `/` to double up the slash, which will produce + } } \keyword{internal} diff --git a/man/provision.Rd b/man/provision.Rd index ffaf68ddb..e42e8b9cb 100644 --- a/man/provision.Rd +++ b/man/provision.Rd @@ -5,7 +5,7 @@ \alias{provision_extra_template} \title{Provision an aggregate page in a lesson} \usage{ -provision_agg_page(pkg, title = "Key Points", slug = "key-points", quiet) +provision_agg_page(pkg, title = "Key Points", slug = "key-points", new = FALSE) provision_extra_template(pkg, quiet = TRUE) } @@ -16,8 +16,9 @@ provision_extra_template(pkg, quiet = TRUE) \item{slug}{the slug for the page (e.g. "aio" will become "aio.html")} -\item{quiet}{if \code{TRUE}, no messages will be emitted. If FALSE, -pkgdown will report creation of the temporary file.} +\item{new}{if \code{TRUE}, (default), the page will be generated +from a new template. If \code{FALSE}, the page is assumed to have been pre-built and +should be appended to.} } \value{ \itemize{ From d68ed9fb54b9aac1fa3ff1f1c175feae79735cd7 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Thu, 21 Apr 2022 18:24:21 -0700 Subject: [PATCH 06/10] attempt to add instructor notes... not well --- R/build_instructor_notes.R | 60 ++++++++++++++++++++++++++++++++++++++ R/build_site.R | 2 ++ R/utils-aggregate.R | 22 ++++++++++++-- R/utils-built-db.R | 2 +- 4 files changed, 82 insertions(+), 4 deletions(-) create mode 100644 R/build_instructor_notes.R diff --git a/R/build_instructor_notes.R b/R/build_instructor_notes.R new file mode 100644 index 000000000..2c1e9ef7d --- /dev/null +++ b/R/build_instructor_notes.R @@ -0,0 +1,60 @@ +build_instructor_notes <- function(pkg, pages = NULL, built = NULL, quiet) { + path <- root_path(pkg$src_path) + this_lesson(path) + outpath <- fs::path(pkg$dst_path, "instructor-notes.html") + already_built <- template_check$valid() && + fs::file_exists(outpath) && + !is.null(built) && + !"instructor-notes" %in% get_slug(built) + if (!already_built) { + page_globals <- setup_page_globals() + inote <- .resources$get()[["instructors"]] + inote <- inote[get_slug(inote) == "instructor-notes"] + html <- render_html(inote) + if (html != '') { + html <- xml2::read_html(html) + fix_nodes(html) + } + + this_dat <- list( + this_page = "instructor-notes.html", + body = use_instructor(html), + pagetitle = "Instructor Notes" + ) + + page_globals$instructor$update(this_dat) + + this_dat$body = use_learner(html) + page_globals$learner$update(this_dat) + + page_globals$meta$update(this_dat) + + build_html(template = "extra", pkg = pkg, nodes = html, + global_data = page_globals, path_md = "instructor-notes.html", quiet = quiet) + } + build_agg_page(pkg = pkg, + pages = pages, + title = this_dat$pagetitle, + slug = "instructor-notes", + aggregate = "/div[contains(@class, 'instructor-note')]//div[@class='accordion-body']", + append = "section[@id='aggregate-instructor-notes']", + prefix = FALSE, + quiet = quiet) +} + +make_instructornotes_section <- function(name, contents, parent) { + title <- names(name) + uri <- sub("^instructor-notes-", "", name) + new_section <- "
+

{title}

+
+
" + section <- xml2::read_xml(glue::glue(new_section)) + for (element in contents) { + for (child in xml2::xml_children(element)) { + xml2::xml_add_child(section, child) + } + xml2::xml_add_child(section, "hr") + } + xml2::xml_add_child(parent, section) +} diff --git a/R/build_site.R b/R/build_site.R index ca6c2f7bb..00f0debfb 100644 --- a/R/build_site.R +++ b/R/build_site.R @@ -105,6 +105,8 @@ build_site <- function(path = ".", quiet = !interactive(), preview = TRUE, overr build_aio(pkg, pages = html_pages, quiet = quiet) if (!quiet) cli::cli_rule(cli::style_bold("Creating Images page")) build_images(pkg, pages = html_pages, quiet = quiet) + if (!quiet) cli::cli_rule(cli::style_bold("Creating Instructor Notes")) + build_instructor_notes(pkg, pages = html_pages, built = built, quiet = quiet) build_sitemap(pkg$dst_path, paths = html_pages$paths, quiet = quiet) diff --git a/R/utils-aggregate.R b/R/utils-aggregate.R index 01155be88..7a3416e3e 100644 --- a/R/utils-aggregate.R +++ b/R/utils-aggregate.R @@ -164,7 +164,7 @@ section_fun <- function(slug) { #' The default is "section", which will aggregate all sections, but nothing #' outside of the sections. To grab everything in the page, use "*" #' @param append a selector for the section of the page where the aggregate data -#' should be placed. This defaults to "self::*", which indicates that the +#' should be placed. This defaults to "self::node()", which indicates that the #' entire page should be appended. #' @param prefix flag to add a prefix for the aggregated sections. Defaults to #' `FALSE`. @@ -202,11 +202,12 @@ section_fun <- function(slug) { #' build_aio(pkg, htmls, quiet = FALSE) #' build_keypoints(pkg, htmls, quiet = FALSE) #' } -build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "section", append = "self::*", prefix = FALSE, quiet = FALSE) { +build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "section", append = "self::node()", prefix = FALSE, quiet = FALSE) { path <- root_path(pkg$src_path) out_path <- pkg$dst_path this_lesson(path) - new_content <- append == "self::*" + + new_content <- append == "self::node()" || append == "self::*" agg <- provision_agg_page(pkg, title = title, slug = slug, new = new_content) if (agg$needs_episodes) { remove_fix_node(agg$learner, slug) @@ -217,6 +218,21 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s learn_parent <- get_content(agg$learner, content = append) instruct_parent <- get_content(agg$instructor, content = append) + needs_content <- !new_content && length(instruct_parent) == 0 + if (needs_content) { + # When the content requested does not exist, we append a new section with + # the id of aggregate-{slug} + sid <- paste0("aggregate-", slug) + learn_parent <- xml2::xml_add_child( + get_content(agg$learner, content = "self::node()"), + "section", id = sid) + instruct_parent <- xml2::xml_add_child( + get_content(agg$instructor, content = "self::node()"), + "section", id = sid) + } + # clean up any content that currently exists + xml2::xml_remove(xml2::xml_child(learn_parent)) + xml2::xml_remove(xml2::xml_child(instruct_parent)) the_episodes <- .resources$get()[["episodes"]] the_slugs <- get_slug(the_episodes) diff --git a/R/utils-built-db.R b/R/utils-built-db.R index 09c2325ca..d36a0b9db 100644 --- a/R/utils-built-db.R +++ b/R/utils-built-db.R @@ -42,7 +42,7 @@ get_built_db <- function(db = "site/built/md5sum.txt", filter = "*R?md") { #' @seealso [get_built_db()] reserved_db <- function(db) { reserved <- c("index", "README", "CONTRIBUTING", "learners/setup", - "profiles[/].*", "links") + "profiles[/].*", "instructors[/]instructor-notes[.]*", "links") reserved <- paste(reserved, collapse = "|") reserved <- paste0("^(", reserved, ")[.]R?md") db[!grepl(reserved, db$file, perl = TRUE), , drop = FALSE] From 7c073b806e686ecf8cd4666202accb103a60eff9 Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 22 Apr 2022 11:38:50 -0700 Subject: [PATCH 07/10] fix instructor notes building --- R/build_instructor_notes.R | 9 +++- R/utils-aggregate.R | 23 +++++----- man/build_agg.Rd | 4 +- tests/testthat/test-build_lesson.R | 63 +++++++++++++++++++++++++-- tests/testthat/test-utils-aggregate.R | 3 +- 5 files changed, 84 insertions(+), 18 deletions(-) diff --git a/R/build_instructor_notes.R b/R/build_instructor_notes.R index 2c1e9ef7d..6a54cd101 100644 --- a/R/build_instructor_notes.R +++ b/R/build_instructor_notes.R @@ -30,7 +30,7 @@ build_instructor_notes <- function(pkg, pages = NULL, built = NULL, quiet) { page_globals$meta$update(this_dat) build_html(template = "extra", pkg = pkg, nodes = html, - global_data = page_globals, path_md = "instructor-notes.html", quiet = quiet) + global_data = page_globals, path_md = "instructor-notes.html", quiet = TRUE) } build_agg_page(pkg = pkg, pages = pages, @@ -43,6 +43,13 @@ build_instructor_notes <- function(pkg, pages = NULL, built = NULL, quiet) { } make_instructornotes_section <- function(name, contents, parent) { + # Since we have hidden the instructor notes from the learner sections, + # there is no point to iterate here, so we return early. + the_call <- match.call() + is_learner <- endsWith(as.character(the_call[["contents"]]), "learn") + if (is_learner) { + return(invisible(NULL)) + } title <- names(name) uri <- sub("^instructor-notes-", "", name) new_section <- "
diff --git a/R/utils-aggregate.R b/R/utils-aggregate.R index 7a3416e3e..a257807f8 100644 --- a/R/utils-aggregate.R +++ b/R/utils-aggregate.R @@ -223,16 +223,17 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s # When the content requested does not exist, we append a new section with # the id of aggregate-{slug} sid <- paste0("aggregate-", slug) - learn_parent <- xml2::xml_add_child( - get_content(agg$learner, content = "self::node()"), - "section", id = sid) - instruct_parent <- xml2::xml_add_child( - get_content(agg$instructor, content = "self::node()"), - "section", id = sid) + learn_content <- get_content(agg$learner, content = "self::node()") + xml2::xml_add_child(learn_content, "section", id = sid) + learn_parent <- xml2::xml_child(learn_content, xml2::xml_length(learn_content)) + + instruct_content <- get_content(agg$instructor, content = "self::node()") + xml2::xml_add_child(instruct_content, "section", id = sid) + instruct_parent <- xml2::xml_child(instruct_content, xml2::xml_length(instruct_content)) } # clean up any content that currently exists - xml2::xml_remove(xml2::xml_child(learn_parent)) - xml2::xml_remove(xml2::xml_child(instruct_parent)) + xml2::xml_remove(xml2::xml_children(learn_parent)) + xml2::xml_remove(xml2::xml_children(instruct_parent)) the_episodes <- .resources$get()[["episodes"]] the_slugs <- get_slug(the_episodes) @@ -257,12 +258,12 @@ build_agg_page <- function(pkg, pages, title = NULL, slug = NULL, aggregate = "s learn_out <- fs::path(out_path, as_html(slug)) instruct_out <- fs::path(out_path, as_html(slug, instructor = TRUE)) report <- "Writing '{.file {out}}'" - out <- fs::path_rel(learn_out, pkg$dst_path) - if (!quiet) cli::cli_text(report) - writeLines(as.character(agg$learner), learn_out) out <- fs::path_rel(instruct_out, pkg$dst_path) if (!quiet) cli::cli_text(report) writeLines(as.character(agg$instructor), instruct_out) + out <- fs::path_rel(learn_out, pkg$dst_path) + if (!quiet) cli::cli_text(report) + writeLines(as.character(agg$learner), learn_out) } #' Get sections from an episode's HTML page diff --git a/man/build_agg.Rd b/man/build_agg.Rd index 1d3137ca5..c719e1094 100644 --- a/man/build_agg.Rd +++ b/man/build_agg.Rd @@ -20,7 +20,7 @@ build_agg_page( title = NULL, slug = NULL, aggregate = "section", - append = "self::*", + append = "self::node()", prefix = FALSE, quiet = FALSE ) @@ -43,7 +43,7 @@ The default is "section", which will aggregate all sections, but nothing outside of the sections. To grab everything in the page, use "*"} \item{append}{a selector for the section of the page where the aggregate data -should be placed. This defaults to "self::*", which indicates that the +should be placed. This defaults to "self::node()", which indicates that the entire page should be appended.} \item{prefix}{flag to add a prefix for the aggregated sections. Defaults to diff --git a/tests/testthat/test-build_lesson.R b/tests/testthat/test-build_lesson.R index e780754f1..e5fb9df34 100644 --- a/tests/testthat/test-build_lesson.R +++ b/tests/testthat/test-build_lesson.R @@ -13,10 +13,32 @@ test_that("Lessons built for the first time are noisy", { expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "ordinary text without R code") }) + pkg <- pkgdown::as_pkgdown(fs::path_dir(sitepath)) + htmls <- read_all_html(sitepath) + expect_setequal(names(htmls$learner), + c("01-introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", + "instructor-notes", "key-points", "aio", "images") + ) + expect_setequal(names(htmls$instructor), + c("01-introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", + "instructor-notes", "key-points", "aio", "images") + ) }) -test_that("aio page exists", { +test_that("build_lesson() also builds the extra pages", { + expect_true(fs::dir_exists(sitepath)) + expect_true(fs::file_exists(fs::path(sitepath, "instructor-notes.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "instructor", "instructor-notes.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "key-points.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "instructor", "key-points.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "aio.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "instructor", "aio.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "images.html"))) + expect_true(fs::file_exists(fs::path(sitepath, "instructor", "images.html"))) +}) + +test_that("aio page can be rebuilt", { skip_if_not(rmarkdown::pandoc_available("2.11")) aio <- fs::path(sitepath, "aio.html") @@ -24,11 +46,46 @@ test_that("aio page exists", { expect_true(fs::file_exists(aio)) expect_true(fs::file_exists(iaio)) html <- xml2::read_html(aio) - content <- xml2::xml_find_all(html, - ".//div[contains(@class, 'lesson-content')]/section[starts-with(@id, 'aio-')]") + content <- get_content(html, "section[starts-with(@id, 'aio-')]") expect_length(content, 1L) expect_equal(xml2::xml_attr(content, "id"), "aio-01-introduction") + # add an ephemeral section and write it out + xml2::xml_add_sibling(content[[1]], "section", id = "ephemeral") + writeLines(as.character(html), aio) + content <- get_content(aio, "section[@id='ephemeral']", pkg = pkg) + expect_length(content, 1L) + + # rebuild the content and check if the section still exists... it shouldn't + build_aio(pkg, pages = htmls, quiet = TRUE) + content <- get_content(aio, "section[@id='ephemeral']", pkg = pkg) + expect_length(content, 0L) + +}) + +test_that("keypoints page can be rebuilt", { + + skip_if_not(rmarkdown::pandoc_available("2.11")) + keypoints <- fs::path(sitepath, "key-points.html") + ikeypoints <- fs::path(sitepath, "instructor/key-points.html") + expect_true(fs::file_exists(keypoints)) + expect_true(fs::file_exists(ikeypoints)) + html <- xml2::read_html(keypoints) + content <- get_content(html, "section[starts-with(@id, 'keypoints-')]") + expect_length(content, 1L) + expect_equal(xml2::xml_attr(content, "id"), "01-introduction") + + # add an ephemeral section and write it out + xml2::xml_add_sibling(content[[1]], "section", id = "ephemeral") + writeLines(as.character(html), keypoints) + content <- get_content(keypoints, "section[@id='ephemeral']", pkg = pkg) + expect_length(content, 1L) + + # rebuild the content and check if the section still exists... it shouldn't + build_keypoints(pkg, pages = htmls, quiet = TRUE) + content <- get_content(keypoints, "section[@id='ephemeral']", pkg = pkg) + expect_length(content, 0L) + }) test_that("sitemap exists", { diff --git a/tests/testthat/test-utils-aggregate.R b/tests/testthat/test-utils-aggregate.R index 0a6fd7887..74ac4be14 100644 --- a/tests/testthat/test-utils-aggregate.R +++ b/tests/testthat/test-utils-aggregate.R @@ -1,4 +1,3 @@ - test_that("read_all_html returns appropriate files", { tmpdir <- withr::local_tempdir() @@ -16,3 +15,5 @@ test_that("read_all_html returns appropriate files", { }) + + From f18b2021607f5931d9d5e0bae23922d73ef7aebf Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 22 Apr 2022 11:52:22 -0700 Subject: [PATCH 08/10] update some tests --- tests/testthat/test-build_lesson.R | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/testthat/test-build_lesson.R b/tests/testthat/test-build_lesson.R index e5fb9df34..7f1f4bd79 100644 --- a/tests/testthat/test-build_lesson.R +++ b/tests/testthat/test-build_lesson.R @@ -13,8 +13,6 @@ test_that("Lessons built for the first time are noisy", { expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "ordinary text without R code") }) - pkg <- pkgdown::as_pkgdown(fs::path_dir(sitepath)) - htmls <- read_all_html(sitepath) expect_setequal(names(htmls$learner), c("01-introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", "instructor-notes", "key-points", "aio", "images") @@ -26,6 +24,9 @@ test_that("Lessons built for the first time are noisy", { }) +htmls <- read_all_html(sitepath) +pkg <- pkgdown::as_pkgdown(fs::path_dir(sitepath)) + test_that("build_lesson() also builds the extra pages", { expect_true(fs::dir_exists(sitepath)) expect_true(fs::file_exists(fs::path(sitepath, "instructor-notes.html"))) @@ -71,7 +72,7 @@ test_that("keypoints page can be rebuilt", { expect_true(fs::file_exists(keypoints)) expect_true(fs::file_exists(ikeypoints)) html <- xml2::read_html(keypoints) - content <- get_content(html, "section[starts-with(@id, 'keypoints-')]") + content <- get_content(html, "section") expect_length(content, 1L) expect_equal(xml2::xml_attr(content, "id"), "01-introduction") From 2c641b379cde2a83c4aa4dd337a25b2c9d606aad Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 22 Apr 2022 12:16:19 -0700 Subject: [PATCH 09/10] add instructor notes test --- tests/testthat/test-build_lesson.R | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tests/testthat/test-build_lesson.R b/tests/testthat/test-build_lesson.R index 7f1f4bd79..d24876b16 100644 --- a/tests/testthat/test-build_lesson.R +++ b/tests/testthat/test-build_lesson.R @@ -13,6 +13,7 @@ test_that("Lessons built for the first time are noisy", { expect_output(build_lesson(tmp, preview = FALSE, quiet = FALSE), "ordinary text without R code") }) + htmls <- read_all_html(sitepath) expect_setequal(names(htmls$learner), c("01-introduction", "index", "LICENSE", "CODE_OF_CONDUCT", "profiles", "instructor-notes", "key-points", "aio", "images") @@ -89,6 +90,33 @@ test_that("keypoints page can be rebuilt", { }) +test_that("instructor-notes page can be rebuilt", { + + skip_if_not(rmarkdown::pandoc_available("2.11")) + notes <- fs::path(sitepath, "instructor-notes.html") + inotes <- fs::path(sitepath, "instructor/instructor-notes.html") + expect_true(fs::file_exists(notes)) + expect_true(fs::file_exists(inotes)) + html <- xml2::read_html(inotes) + content <- get_content(html, "section[@id='aggregate-instructor-notes']/section") + expect_length(content, 1L) + expect_equal(xml2::xml_attr(content, "id"), "01-introduction") + expect_match(xml2::xml_text(xml2::xml_find_first(content[[1]], ".//p")), + "Inline instructor notes") + + # add an ephemeral section and write it out + xml2::xml_add_sibling(content[[1]], "section", id = "ephemeral") + writeLines(as.character(html), inotes) + content <- get_content(inotes, "/section[@id='ephemeral']", pkg = pkg, instructor = TRUE) + expect_length(content, 1L) + + # rebuild the content and check if the section still exists... it shouldn't + build_instructor_notes(pkg, pages = htmls, quiet = TRUE) + content <- get_content(inotes, "/section[@id='ephemeral']", pkg = pkg, instructor = TRUE) + expect_length(content, 0L) + +}) + test_that("sitemap exists", { skip_if_not(rmarkdown::pandoc_available("2.11")) sitemap <- fs::path(sitepath, "sitemap.xml") From 30d59b219d6d192363946d2335ee4241923438af Mon Sep 17 00:00:00 2001 From: "Zhian N. Kamvar" Date: Fri, 22 Apr 2022 12:32:47 -0700 Subject: [PATCH 10/10] add NEWS; update docs; bump version --- DESCRIPTION | 2 +- NEWS.md | 22 +++++++++++++ R/build_instructor_notes.R | 36 +++++++++++++++++++++ man/build_agg.Rd | 8 ++++- man/make_instructornotes_section.Rd | 50 +++++++++++++++++++++++++++++ 5 files changed, 116 insertions(+), 2 deletions(-) create mode 100644 man/make_instructornotes_section.Rd diff --git a/DESCRIPTION b/DESCRIPTION index d045aceaf..6f069c7a0 100644 --- a/DESCRIPTION +++ b/DESCRIPTION @@ -1,6 +1,6 @@ Package: sandpaper Title: Create and Curate Carpentries Lessons -Version: 0.4.1 +Version: 0.5.0 Authors@R: c( person(given = "Zhian N.", family = "Kamvar", diff --git a/NEWS.md b/NEWS.md index 81dcc6d43..040ea2e93 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,25 @@ +# sandpaper 0.5.0 + +NEW FEATURES +------------ + + * `images.html` is built with internal function `build_images()`, collecting + all images and displaying alt text for non-screen reader users (while + marking those paragraphs as `aria-hidden` so that screen reader users do not + have it read twice). + * `instructor-notes.html` is now built with the internal function + `build_instructor_notes()` and now collects instructor notes from the + episodes in a section called `aggregate-instructor-notes`. + +MISC +---- + + * The internal `build_agg_page()` has a new argument, `append`, which takes an + XPath expression of what node should have children appended. Defaults to + `"self::node()"`. An example of alternate usage is in + `build_instructor_notes()`, which uses + `section[@id='aggregate-instructor-notes']`. + # sandpaper 0.4.1 MISC diff --git a/R/build_instructor_notes.R b/R/build_instructor_notes.R index 6a54cd101..21bf9974b 100644 --- a/R/build_instructor_notes.R +++ b/R/build_instructor_notes.R @@ -1,3 +1,6 @@ +#' @rdname build_agg +#' @param built a vector of markdown documents that have recently been rebuilt +#' (for future use) build_instructor_notes <- function(pkg, pages = NULL, built = NULL, quiet) { path <- root_path(pkg$src_path) this_lesson(path) @@ -42,6 +45,39 @@ build_instructor_notes <- function(pkg, pages = NULL, built = NULL, quiet) { quiet = quiet) } +#' Make a section of aggregated instructor notes +#' +#' This will append instructor notes from the inline sections of the lesson to +#' the instructor-notes page, separated by section and `
` elements. +#' +#' @param name the name of the section, (may or may not be prefixed with `images-`) +#' @param contents an `xml_nodeset` of figure elements from [get_content()] +#' @param parent the parent div of the images page +#' @return the section that was added to the parent +#' @note On the learner view, instructor notes will not be present +#' +#' @keywords internal +#' @seealso [build_instructor_notes()], [get_content()] +#' @examples +#' if (FALSE) { +#' lsn <- "/path/to/lesson" +#' pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) +#' +#' # read in the All in One page and extract its content +#' notes <- get_content("instructor-notes", content = +#' "section[@id='aggregate-instructor-notes']", pkg = pkg, instructor = TRUE) +#' agg <- "/div[contains(@class, 'instructor-note')]//div[@class='accordion-body']" +#' note_content <- get_content("01-introduction", content = agg, pkg = pkg) +#' make_instructornotes_section("01-introduction", contents = note_content, +#' parent = notes) +#' +#' # NOTE: if the object for "contents" ends with "_learn", no content will be +#' # appended +#' note_learn <- note_content +#' make_instructornotes_section("01-introduction", contents = note_learn, +#' parent = notes) +#' +#' } make_instructornotes_section <- function(name, contents, parent) { # Since we have hidden the instructor notes from the learner sections, # there is no point to iterate here, so we return early. diff --git a/man/build_agg.Rd b/man/build_agg.Rd index c719e1094..3fcbe13af 100644 --- a/man/build_agg.Rd +++ b/man/build_agg.Rd @@ -1,9 +1,10 @@ % Generated by roxygen2: do not edit by hand % Please edit documentation in R/build_aio.R, R/build_images.R, -% R/build_keypoints.R, R/utils-aggregate.R +% R/build_instructor_notes.R, R/build_keypoints.R, R/utils-aggregate.R \name{build_aio} \alias{build_aio} \alias{build_images} +\alias{build_instructor_notes} \alias{build_keypoints} \alias{build_agg_page} \title{Build a page for aggregating common elements} @@ -12,6 +13,8 @@ build_aio(pkg, pages = NULL, quiet = FALSE) build_images(pkg, pages = NULL, quiet = FALSE) +build_instructor_notes(pkg, pages = NULL, built = NULL, quiet) + build_keypoints(pkg, pages = NULL, quiet = FALSE) build_agg_page( @@ -34,6 +37,9 @@ build_agg_page( \item{quiet}{if \code{TRUE}, no messages will be emitted. If FALSE, pkgdown will report creation of the temporary file.} +\item{built}{a vector of markdown documents that have recently been rebuilt +(for future use)} + \item{title}{the new page title} \item{slug}{the slug for the page (e.g. "aio" will become "aio.html")} diff --git a/man/make_instructornotes_section.Rd b/man/make_instructornotes_section.Rd new file mode 100644 index 000000000..a409627c3 --- /dev/null +++ b/man/make_instructornotes_section.Rd @@ -0,0 +1,50 @@ +% Generated by roxygen2: do not edit by hand +% Please edit documentation in R/build_instructor_notes.R +\name{make_instructornotes_section} +\alias{make_instructornotes_section} +\title{Make a section of aggregated instructor notes} +\usage{ +make_instructornotes_section(name, contents, parent) +} +\arguments{ +\item{name}{the name of the section, (may or may not be prefixed with \verb{images-})} + +\item{contents}{an \code{xml_nodeset} of figure elements from \code{\link[=get_content]{get_content()}}} + +\item{parent}{the parent div of the images page} +} +\value{ +the section that was added to the parent +} +\description{ +This will append instructor notes from the inline sections of the lesson to +the instructor-notes page, separated by section and \verb{
} elements. +} +\note{ +On the learner view, instructor notes will not be present +} +\examples{ +if (FALSE) { +lsn <- "/path/to/lesson" +pkg <- pkgdown::as_pkgdown(fs::path(lsn, "site")) + +# read in the All in One page and extract its content +notes <- get_content("instructor-notes", content = + "section[@id='aggregate-instructor-notes']", pkg = pkg, instructor = TRUE) +agg <- "/div[contains(@class, 'instructor-note')]//div[@class='accordion-body']" +note_content <- get_content("01-introduction", content = agg, pkg = pkg) +make_instructornotes_section("01-introduction", contents = note_content, + parent = notes) + +# NOTE: if the object for "contents" ends with "_learn", no content will be +# appended +note_learn <- note_content +make_instructornotes_section("01-introduction", contents = note_learn, + parent = notes) + +} +} +\seealso{ +\code{\link[=build_instructor_notes]{build_instructor_notes()}}, \code{\link[=get_content]{get_content()}} +} +\keyword{internal}