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

Attach component-specific Sass/Css at the component level; allow for more customization via CSS variables #664

Merged
merged 20 commits into from Jul 11, 2023

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Jul 10, 2023

Currently, bs_theme() imports Sass files for bslib components (e.g., cards, sidebars, etc), which has a couple benefits:

  1. Allows us to leverage Sass features like @extend and placeholders that, in effect, modify Bootstrap.
  2. Minimizes sass::sass() calls (since sass may need to be re-compiled for each component).
  3. Reduces complexity in the HTMLDependency() of these components.

However, there are downsides that outweigh those benefits:

  1. Components currently don't work in Quarto (Quarto HTML incompatible with bslib full screen cards quarto-dev/quarto-cli#6081). This is because Quarto brings it's own Bootstrap dependency. It actually does this by copying over bslib's patched version of Bootstrap, so it could, in theory, also bring over the component Sass, but it feels wrong to increase the potential for issues brought about by incompatibility between build-time vs run-time dependencies.
  2. Generally it's a bit weird and confusing to bundle these components with the core Bootstrap bundle.
  3. We're shipping more CSS than needed for some apps (this will continue to get worse as we build out more components)

This PR moves component-level Sass out of bs_theme() and into the relevant components. And since do might need to re-compile Sass in those situations, it follows the dynamically theme-able component model.

Also, while we're here, I took the opportunity to reduce the amount of Sass specific calculations (by moving what we can to CSS), and more fully embrace CSS variables for customization. We'll still keep Sass around to leverage things like mix() and color-contrast(), but once CSS has good support for these things, we could potentially remove the (run-time) Sass dependency for these components, and still allow them to be theme-able (by allowing defaults to derive from Bootstrap's CSS variables)

R/utils-deps.R Outdated Show resolved Hide resolved
@cpsievert cpsievert marked this pull request as ready for review July 10, 2023 21:44
@cpsievert cpsievert requested a review from gadenbuie July 10, 2023 21:45
R/accordion.R Outdated Show resolved Hide resolved
R/card.R Outdated Show resolved Hide resolved
R/page.R Outdated Show resolved Hide resolved
R/sidebar.R Outdated Show resolved Hide resolved
R/utils-deps.R Outdated Show resolved Hide resolved
@cpsievert cpsievert merged commit 8b0dc57 into main Jul 11, 2023
12 checks passed
@cpsievert cpsievert deleted the componentize-scss branch July 11, 2023 19:30
cpsievert added a commit that referenced this pull request Jul 11, 2023
…sn't create an empty card-body (i.e., card item)
cpsievert added a commit that referenced this pull request Jul 13, 2023
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.

None yet

2 participants