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

Slider handle is in wrong location in Crosstalk example #813

Closed
wch opened this issue Oct 2, 2023 · 6 comments · Fixed by #815
Closed

Slider handle is in wrong location in Crosstalk example #813

wch opened this issue Oct 2, 2023 · 6 comments · Fixed by #815
Assignees

Comments

@wch
Copy link
Collaborator

wch commented Oct 2, 2023

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

The slider looks like this:

image

This is probably related to rstudio/shiny#3905.

@gadenbuie
Copy link
Member

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/shiny/blob/main/inst/www/shared/ionrangeslider/scss/shiny.scss
$emphasis-color-rgb: var(--bs-emphasis-color-rgb, 0, 0, 0) !default;
$minmax_bg_color: RGBA($emphasis-color-rgb, 0.1) !default;
$top: 32px !default;

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
Copy link
Member

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

bslib/R/bs-dependencies.R

Lines 345 to 346 in ad946ca

# Outside of a Shiny context, we'll just get the global theme.
mfunc(bs_global_get())

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

theme <- bs_theme(5, preset = "shiny")
bs_global_set(theme)

@cpsievert
Copy link
Collaborator

cpsievert commented Oct 3, 2023

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 rstudio/htmltools#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
Copy link
Member

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
Copy link
Member

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

@gadenbuie gadenbuie self-assigned this Oct 4, 2023
Copy link

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.
🙋 Need help? Connect with us on Discord or Posit Community.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants