-
Notifications
You must be signed in to change notification settings - Fork 57
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
refactor(theme-preset): Streamline S3 classes, store theme info in sass variables #644
Conversation
* Themes with presets now have consistent classes: `bs_theme_with_prest` and `bs_preset_bultin` or `bs_preset_bootswatch`. * Preset name and type are stored in `$bslib-preset-name` and `$bslib-preset-type` * `theme_preset_info()` returns all preset information, including bootstrap version
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
R/bs-theme.R
Outdated
if (!is.null(preset$theme_class)) { | ||
bundle <- add_class(bundle, preset$theme_class) | ||
} |
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 still doesn't feel quite right that $theme_class
exists. Could we get rid of it, and do something like this?
if (!is.null(preset$theme_class)) { | |
bundle <- add_class(bundle, preset$theme_class) | |
} | |
if (!is.null(preset$type)) { | |
bundle <- add_class(bundle, theme_preset_class()) | |
} |
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 don't mind, I made the change to a constant in 3c44cc3.
The reason I didn't originally was because it felt like preset$type
being NULL was an indirect signal of whether this class should be added. I included preset$theme_class
so that if we changed that assumption later we'd only need to reason about the preset
object.
But the problem is that we have to remove preset the preset class as part of bs_theme_update()
... so I'm on board with constant as a the signal that we're doing something special.
… in the preset" This reverts commit 1a602d2.
This PR improves our use of S3 classes around preset themes:
bs_preset_bundle()
takes a preset object but doesn't use S3 for dispatching. If it's not apreset
or ifpreset$type
is missing, the bundle returnsNULL
.bs_theme()
, when a preset is added to the theme, returns an object with classbs_theme_with_preset
, in addition to a version-specific class. To determine the preset name and type, calltheme_preset_info()
to read the values from the sass bundle.The preset bundles now include two SASS variables,
$bslib-preset-name
and$bslib-prest-type
, that encode the name and type of the preset. We already included$bootstrap-version
, so it's now possible to completely extract the theme information from the SASS bundles. I addedtheme_preset_info()
that does just this.bs_version_N
orbs_theme_with_preset
classes before making the more expensive call tobs_get_variables()
.bs_theme_init()
so they're guaranteed to exist for allbs_theme()
-created themes. I've also added rules that create CSS variables, e.g.--bslib-bootstrap-version
and--bslib-prest-name
etc, to allow us to know the Bootstrap version and our preset values even in the compiled CSS.