rstudio / bslib

Tools for theming Shiny and R Markdown via Bootstrap 3, 4, or 5.
https://rstudio.github.io/bslib/
Other
469 stars 57 forks source link

Slider handle is in wrong location in Crosstalk example #813

Closed wch closed 11 months ago

wch commented 11 months ago

On this page https://rstudio.github.io/bslib/articles/any-project/

The slider looks like this:

image

This is probably related to https://github.com/rstudio/shiny/issues/3905.

gadenbuie commented 11 months ago

The key problem here appears to be that we're getting the compiled ion.rangeSlider.css file from Shiny rather than the dynamic dependency that involves the Sass compilation step (see https://github.com/rstudio/shiny/blob/a6fc6bf8cb5032138fb33327d07ad0f4e19c9f86/R/input-slider.R#L225-L235)

The result is that we're missing the effects of the Sass variables that are set in the shiny preset for ionrangeslider https://github.com/rstudio/bslib/blob/ad946cafdbf1c91cfd714cb39948bb203ae66e66/inst/builtin/bs5/shiny/ionrangeslider/_variables.scss#L1-L7

I think part of this regression might be an inadvertent side-effect of moving from opting into the shiny preset to having it be the default.

@cpsievert Any ideas about how to proceed?

gadenbuie commented 11 months ago

Specifically, in the deferred dependency evaluation, bs_global_get() returns NULL for preset = "shiny" here

https://github.com/rstudio/bslib/blob/ad946cafdbf1c91cfd714cb39948bb203ae66e66/R/bs-dependencies.R#L345-L346

but the styles are fixed by adding these lines to the example app

theme <- bs_theme(5, preset = "shiny")
bs_global_set(theme)
cpsievert commented 11 months ago

Any ideas about how to proceed?

Ideally we'd generally solve this class of issue by allowing bs_dependency_defer() to identify the correct theme value when statically rendered. A long time ago I had looked into doing this (via https://github.com/rstudio/htmltools/pull/267), but I can't remember exactly why we dropped it (other than it being a difficult thing to get right). Although, even if we did solve this problem, it wouldn't improve the equivalent Quarto scenario, so in the short term, I'm leaning towards either: (1) making these styling changes via rules (not variables) or (2) updating the ionrangeslider sass to make use of CSS variables (and having the preset change the defaults). Definitely in the long term, it would make sense to do 2, but it's also not ideal for us to now require a new version of shiny to get the better looking slider.

gadenbuie commented 11 months ago

I'd rather solve the problem of bs_dependency_defer() not resolving the correct theme value when statically rendered. I wonder if the problem might get easier if we thought of it not as a general problem that needs to be solved in the abstract for any tag() or tagList(), but a problem specific to the page_*() functions in bslib (where theme is set)?

gadenbuie commented 11 months ago

815 has a proposal for temporarily setting the global theme during render via the bslib_page objects returned by our page_*() functions.

github-actions[bot] commented 9 months ago

This issue has been automatically locked. If you have found a related problem, please open a new issue (with a reproducible example or feature request) and link to this issue. :raising_hand: Need help? Connect with us on Discord or Posit Community.