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 force-pushed the componentize-scss branch from 62958c0 to 07c9032 Compare July 10, 2023 19:39
@cpsievert cpsievert force-pushed the componentize-scss branch from 07c9032 to ce6cad0 Compare July 10, 2023 19:41
@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
@cpsievert cpsievert force-pushed the componentize-scss branch from f9ad3ce to f3e3b15 Compare July 10, 2023 22:28
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
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