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.

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

Merged
merged 24 commits into from
Jun 26, 2023

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Jun 20, 2023

This PR improves our use of S3 classes around preset themes:

  1. bs_preset_bundle() takes a preset object but doesn't use S3 for dispatching. If it's not a preset or if preset$type is missing, the bundle returns NULL.

  2. bs_theme(), when a preset is added to the theme, returns an object with class bs_theme_with_preset, in addition to a version-specific class. To determine the preset name and type, call theme_preset_info() to read the values from the sass bundle.

  3. 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 added theme_preset_info() that does just this.

    • Extracting variables from the SASS layers is slow enough that I think it's worth testing for the presence of bs_version_N or bs_theme_with_preset classes before making the more expensive call to bs_get_variables().
    • The SASS variables are now defined in bs_theme_init() so they're guaranteed to exist for all bs_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.

* 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
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme.R Outdated Show resolved Hide resolved
@gadenbuie gadenbuie requested a review from cpsievert June 21, 2023 15:13
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme.R Outdated Show resolved Hide resolved
@gadenbuie gadenbuie requested a review from cpsievert June 21, 2023 18:54
R/bs-theme.R Outdated Show resolved Hide resolved
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme-preset.R Outdated Show resolved Hide resolved
R/bs-theme-preset.R Outdated Show resolved Hide resolved
@gadenbuie gadenbuie requested a review from cpsievert June 21, 2023 21:34
R/bs-theme.R Outdated Show resolved Hide resolved
R/bs-theme.R Outdated
Comment on lines 136 to 138
if (!is.null(preset$theme_class)) {
bundle <- add_class(bundle, preset$theme_class)
}
Copy link
Collaborator

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?

Suggested change
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())
}

Copy link
Member Author

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.

@gadenbuie gadenbuie requested a review from cpsievert June 22, 2023 16:31
@gadenbuie gadenbuie merged commit 10dddd0 into main Jun 26, 2023
12 checks passed
@gadenbuie gadenbuie deleted the refresh/preset-theme-classes branch June 26, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants