-
-
Notifications
You must be signed in to change notification settings - Fork 978
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.
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
base: main
Are you sure you want to change the base?
Conversation
91a955e
to
116907c
Compare
Hi @cpsievert - This works nicely for me- thank you! I'm confused about how to change the text color though. I have:
But in my knitted slides, the text is:
I am new to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all this @cpsievert !
First let me say, It was a great way for me to dive on how bslib and sass integrates with a format. I think I understand it better now but it is not yet perfect. So, just to clarify for me: We don't really use bootstrap 4 here but only some of its variables that were integrated into the default.scss
so that we can render a custom css. Is that right ? Would sass alone would have been enough to achieve the same ? But we use bs_dependency
here, so does this mean that all boostrap is available in ioslide document ?
Back to the review: the comments I made below are mainly feedback to discuss. ioslides_presentation
does not currently provide a theme
argument, so I think we need to take care of checking that it is not to be confuse with the one from other format. In the case of ioslides, it mainly allow to pass variables to change the default CSS using bslib features. We should prevent / guide against the known usage for other formats usually for BS3. Also, it differs a bit with html_document
because you can only pass a subset of variables - maybe we should be more verbose to guide users.
Regarding tests, I did with simple examples for now and it works well. I am not yet used to bootstrap variable, so it is not easy to see which variables change what, but the shiny app helps. I only found one difference in CSS not related to color so I have a question. (See below)
I assume you have tested this on several example documents like for html_document
?
When using thematic_shiny
with your example, I have a bunch of warning in the R Markdown pane:
WARNING: Found no color leading to 3:1 contrast ratio against #F46D8D...
on line 41:5 of ../../R/win-library/4.0/bslib/sass-utils/color-contrast.scss, in function `color-contrast`
from line 7:10 of ../../R/win-library/4.0/bslib/lib/bs/scss/mixins/_buttons.scss, in mixin `button-variant`
from line 75:14 of ../../R/win-library/4.0/bslib/lib/bs/scss/_buttons.scss
from line 37:9 of stdin
Is this something related to this PR ? I installed the last dev version of bslib, thematic and sass.
Anyway, thanks for this PR and let's discuss the suggested changes and questions below.
#' \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) |
There was a problem hiding this comment.
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.
# 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) |
There was a problem hiding this comment.
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
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.
theme <- resolve_theme(theme) | |
theme <- resolve_theme(theme, allow_bs3 = FALSE) |
There was a problem hiding this comment.
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.
theme <- resolve_theme(theme) | ||
if (!is_bs_theme(theme)) { |
There was a problem hiding this comment.
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.
if ("3" %in% theme_version(theme)) { | ||
stop("`ioslides_presentation()` is not compatible with Bootstrap 3 (use version = 4 or higher)") | ||
} |
There was a problem hiding this comment.
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.
|
||
/* line 62, ../scss/default.scss */ | ||
slides > slide { | ||
color: mix($body-bg, $body-color, 47.5%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason to have removed the font-size
here ? Seems like a mistake as you removed font-family
from other rules but not font-size
This cause the default theme to not have the same font size. I just tried for example:
ioslides_presentation:
theme:
code_font: !expr bslib::font_google("JetBrains Mono")
to compare
color: mix($body-bg, $body-color, 47.5%); | |
font-size: 26px; | |
color: mix($body-bg, $body-color, 47.5%); |
@apreshill the CSS rule that will be processed by sass is this one: .title-slide hgroup h1 {
font-size: 65px;
line-height: 1.4;
letter-spacing: -3px;
color: mix($body-bg, $body-color, 32%);
} So color is a mix of What was the color you expected ? Does the mix does not correspond ? As a side note, I tried to test using this code, but it does not output the same color 😅 It must be more complexe than that with other variables in the mix. > sass::sass(
+ list(list(`body-bg` = "#f3e0e1", `body-color` = "#0144a3"),
+ ".title-slide hgroup h1 {
+ font-size: 65px;
+ line-height: 1.4;
+ letter-spacing: -3px;
+ color: mix($body-bg, $body-color, 32%);
+ }"))
/* CSS */
.title-slide hgroup h1 {
font-size: 65px;
line-height: 1.4;
letter-spacing: -3px;
color: #4e76b7;
} |
Yep, this PR is currently tying the ioslide's Sass variables to Bootstrap Sass variables, allowing us to compile the ioslide Sass with Bootstrap's variables. Thus, there is some implied risk that Bootstrap variables will change in a future version, but the variables we're listening to here (
No, but this is a direction worth thinking more seriously about, especially for Bootstrap-less HTML formats like ioslides,
Yea, that was the main thing I was torn about in this PR. That is, currently if you provide a
You're seeing rstudio/bslib#242 |
No problem. We are not in a hurry for this, and I think we need to it right for those Bootstrap-less format to make it easy to use and easy to teach. Let's revisit later then and get some more feedback in the meantime. |
This PR makes
ioslides_presentation()
's CSS more aware ofbslib::bs_theme()
's main colors & fonts. That is, you can now customize the main colors and fonts in the CSS behindioslides_presentation()
in a similar way to how we can now customize the Bootstrap CSS behindhtml_document()
via{bslib}
, for example:This also works with bslib+shiny's "real-time" theming capabilities: