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.

Sign up for GitHub

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

Make ioslides_presentation() more themable via {bslib} and {sass} #2013

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Make ioslides_presentation() more themable via {bslib} and {sass} #2013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3009659
Make ioslides_presentation() more themable via {bslib} and {sass}
cpsievert Jan 20, 2021
116907c
Make code font-family inherit from font-family-monospace
cpsievert Jan 20, 2021
50d178a
make code-color configurable, update news and version
cpsievert Feb 4, 2021
d0b53c2
Use resolve_theme() to determine if bslib is relevant
cpsievert Feb 4, 2021
7403afc
For better bootswatch compatibility, cascade defaults based on body-b…
cpsievert Feb 4, 2021
fd331ac
Make ioslides 'dynamically' themeable
cpsievert Feb 4, 2021
3e81227
Accidentally committed a wip file
cpsievert Feb 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Package: rmarkdown
Type: Package
Title: Dynamic Documents for R
Version: 2.6.6
Version: 2.6.7
Authors@R: c(
person("JJ", "Allaire", role = "aut", email = "jj@rstudio.com"),
person("Yihui", "Xie", role = c("aut", "cre"), email = "xie@yihui.name", comment = c(ORCID = "0000-0003-0645-5666")),
Expand Down
2 changes: 2 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ rmarkdown 2.7

- Files with `.scss`/`.sass` extension (i.e., Sass files) provided to `html_document`'s `css` parameter are now compiled to CSS using the `{sass}` package. Also, if `theme` is a `{bslib}` object, these Sass files may utilize Sass code inside the `theme` (thanks, @cpsievert, #1706)

- ` ioslides_presentation` gains an (explicit) `theme` argument with support for `bslib::bs_theme()` objects/arguments, meaning that one may opt-into Bootstrap 4 and more easily create custom themes. For examples, see <https://github.com/rstudio/rmarkdown/pull/2013> (thanks, @cpsievert, #2013)

- Fix an issue with line numbering in code chunks when `.numberlines` with Pandoc's highlighting (thanks, @aosavi, #1876)

- Fix an issue with shiny runtime and `global.R` (thanks, @liaojiahui-r, rstudio/flexdashboard#298)
Expand Down
79 changes: 63 additions & 16 deletions R/ioslides_presentation.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,12 @@
#' @param transition Speed of slide transitions. This can be "default",
#' "slower", "faster", or a numeric value with a number of seconds (e.g. 0.5).
#' @param analytics A Google analytics property ID.
#'@param smart Produce typographically correct output, converting straight
#' quotes to curly quotes, \code{---} to em-dashes, \code{--} to en-dashes, and
#' \code{...} to ellipses.
#' @param smart Produce typographically correct output, converting straight
#' quotes to curly quotes, \code{---} to em-dashes, \code{--} to en-dashes, and
#' \code{...} to ellipses.
#' @param theme a list of theming arguments (currently `bg`, `fg`, `primary`,
#' `success`, `warning`, `danger`, `base_font`, and `code_font` are supported,
#' see [bslib::bs_theme()] for more about these options)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use yet the markdown syntax by default in this package. So you need to use usual Rd syntax or set @md in this specific doc.

If we do the latter, we may take the opportunity to convert this whole help page.
The former is easier for you as this is one link.

#' @return R Markdown output format to pass to \code{\link{render}}.
#' @details
#' See the \href{https://bookdown.org/yihui/rmarkdown/ioslides-presentation.html}{
Expand Down Expand Up @@ -248,6 +251,7 @@ ioslides_presentation <- function(number_sections = FALSE,
md_extensions = NULL,
pandoc_args = NULL,
extra_dependencies = NULL,
theme = NULL,
...) {

# base pandoc options for all output
Expand Down Expand Up @@ -289,9 +293,17 @@ ioslides_presentation <- function(number_sections = FALSE,
template <- pkg_file("rmd/ioslides/default.html")
args <- c(args, "--template", pandoc_path_arg(template))

# html dependency for ioslides
extra_dependencies <- append(extra_dependencies,
list(html_dependency_ioslides()))
# Add ioslides HTML dependencies
# If bslib is relevant, we want to register a _deferred_ version of the CSS dependency so
# it can update in response to Shiny's session$setCurrentTheme() (e.g., bslib::bs_themer())
# https://rstudio.github.io/bslib/articles/theming.html#dynamic-theming-in-shiny
theme <- resolve_theme(theme)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should provide a more specific handling / checking of the theme argument for this specific format. ioslides_presentation had no argument before that because it did not support a theme as other formats. Introducing this argument may be confusing compared to other format because it will be different: Only a list to pass to bslib will be supported.

For example, using

  ioslides_presentation:
    theme: cerulean

will issue no warning or error. It will do nothing, so not harmful but some users could expect this to work as other formats. (even if the doc does not say so).

Maybe we can check early and warn that will have not effect (or stop and say only bslib is supported so a list.
Something like

Suggested change
theme <- resolve_theme(theme)
if (!is.null(theme) && is.character(theme)) {
stop("With 'ioslides_presentation', 'theme' only works with bslib package and must be a list.", call. = FALSE)
}
theme <- resolve_theme(theme)

We could also modify the resolve_theme function to add enforce_bslib = FALSE argument.
When set to TRUE, it would error if the theme provided is not a list to pass to bslib::bs_theme. (and not just return the match.arg among BS3 theme.

It could also be allow_bs3 = TRUE by default, that would do the above when set to FALSE.

We would have for this format.

Suggested change
theme <- resolve_theme(theme)
theme <- resolve_theme(theme, allow_bs3 = FALSE)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using ioslides_presentation: theme: cerulean will issue no warning or error. It will do nothing, so not harmful but some users could expect this to work as other formats.

This does do something though -- it brings in Bootstrap! So, I don't think we want to introduce an error in this case -- I'm sure folks are relying on this functionality.

if (is_bs_theme(theme)) {
dynamic_ioslides <- bslib::bs_dependency_defer(html_dependency_ioslides)
theme <- bslib::bs_bundle(theme, sass::sass_layer(html_deps = dynamic_ioslides))
} else {
extra_dependencies <- append(extra_dependencies, html_dependency_ioslides(theme))
}

# analytics
if (!is.null(analytics))
Expand Down Expand Up @@ -453,14 +465,16 @@ ioslides_presentation <- function(number_sections = FALSE,
mathjax = mathjax,
pandoc_args = pandoc_args,
extra_dependencies = extra_dependencies,
bootstrap_compatible = TRUE, ...))
bootstrap_compatible = TRUE, theme = theme, ...))
}


html_dependency_ioslides <- function() {
htmlDependency(
name = "ioslides",
version = "13.5.1",
html_dependency_ioslides <- function(theme = NULL) {
name <- "ioslides"
version <- "13.5.1"
js <- htmlDependency(
name = paste0(name, "-js"),
version = version,
src = pkg_file("rmd/ioslides/ioslides-13.5.1"),
script = c(
"js/modernizr.custom.45394.js",
Expand All @@ -470,10 +484,43 @@ html_dependency_ioslides <- function() {
"js/hammer.js",
"js/slide-controller.js",
"js/slide-deck.js"
),
stylesheet = c(
"fonts/fonts.css",
"theme/css/default.css",
"theme/css/phone.css")
)
)

theme <- resolve_theme(theme)
if (!is_bs_theme(theme)) {
Comment on lines +490 to +491
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are already doing this on the outside of the function. As ioslides_presentation is not exported, can't we assume that theme argument is not NULL will always be a bslib compatible theme ?

Aim is just to avoid some duplication of logic in different places.

css <- htmlDependency(
name = paste0(name, "-css"),
version = version,
src = pkg_file("rmd/ioslides/ioslides-13.5.1"),
stylesheet = c(
"fonts/fonts.css",
"theme/css/default.css",
"theme/css/phone.css"
)
)
return(list(js, css))
}

if ("3" %in% theme_version(theme)) {
stop("`ioslides_presentation()` is not compatible with Bootstrap 3 (use version = 4 or higher)")
}
Comment on lines +505 to +507
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel this should be tested in the format function ioslides_presentation directly when we detect bslib is in use.

io_file <- function(...) {
sass::sass_file(pkg_file("rmd/ioslides/ioslides-13.5.1", ...))
}
css <- bslib::bs_dependency(
input = sass::sass_layer(
rules = list(
io_file("fonts/fonts.css"),
io_file("theme/css/default.scss"),
io_file("theme/css/phone.css")
),
file_attachments = c(fonts = pkg_file("rmd/ioslides/ioslides-13.5.1/fonts"))
),
theme = theme,
name = paste0(name, "-css"),
version = version,
cache_key_extra = packageVersion("rmarkdown")
)
list(js, css)
}
Loading