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 when used with Quarto #3905

Closed
wch opened this issue Oct 2, 2023 · 11 comments
Closed

Slider handle is in wrong location when used with Quarto #3905

wch opened this issue Oct 2, 2023 · 11 comments

Comments

@wch
Copy link
Collaborator

wch commented Oct 2, 2023

When using the sliderInput with a different build of Bootstrap (compared to the bootstrap.css in bslib), the slider looks like this:

slider

This is because the CSS for the handle position is in bslib's copy of bootstrap.css:

slider css

This happens when using the slider in a Quarto document.

To fix this, I think the slider styling in bslib somehow needs to be put into Shiny.

@cpsievert
Copy link
Collaborator

Are you bringing the slider in via PyShiny (a reprex would be great)?

@wch
Copy link
Collaborator Author

wch commented Oct 2, 2023

Right, the slider is coming in from Shiny for Python. Sorry, I don't have a simple reprex at this point, since generating the output currently involves some things that are experimental.

I also tried reproducing with Quarto and Shiny for R, but I couldn't figure out how to get it to display sliders with the updated look (with no tick marks). If you can get Quarto and Shiny for R to display the new sliders, I suspect they will look the same.

@wch
Copy link
Collaborator Author

wch commented Oct 2, 2023

I'm not sure if this is exactly the same issue, but it seems like it's probably related: rstudio/bslib#813

@gadenbuie
Copy link
Member

@wch I suspect the issue is fundamentally the same for Python and R -- the static, precompiled CSS for ionrangeslider not using the Sass variables of the new shiny preset -- but the fix will likely be different. In the case of Shiny for Python, I'm not sure how ionrangeslider's CSS dependency is handled, but I suspect the fix will be to re-compile that CSS in the shiny preset context.

@gadenbuie
Copy link
Member

I think we should move this issue into the py-shiny repo (or close it in favor of a new issue); on the R side the work needed is covered by the bslib issue.

@wch
Copy link
Collaborator Author

wch commented Oct 3, 2023

Just to make sure we're on the same page: The slider looks fine in a regular Shiny for Python app, but not when the slider from py-shiny is used in a Quarto doc.

Shiny for Python has a build of bootstrap.min.css which includes the CSS in the screenshot above, for .irs.irs--shiny .irs-handle.

In the Quarto case, the problem is that Quarto's build of bootstrap.min.css is taken from an older version of bslib, and does not have that CSS. As far as I know, there's not a way to cleanly override Quarto's version of Bootstrap. Note that Quarto will load the ion.RangeSlider HTML dependency, so the resulting document uses Shiny's version of ion.RangeSlider.css, with Quarto's version of bootstrap.min.css.

I think Shiny for Python currently simply copies over the CSS files from Shiny for R. If I run a simple example app in R, the browser inspector shows that there is some slider styling is in bootstrap.min.css.

image

And here it is with the .irs.irs--shiny .irs-handle rule in bootstrap.min.css disabled:

image

App code:

library(shiny)
library(bslib)

ui <- fluidPage(
  theme = bs_theme(preset = "shiny"),
  sliderInput("n", "N:", min = 0, max = 1000, value = 500)
)

shinyApp(ui, function(input, output) {})

@gadenbuie
Copy link
Member

In the Quarto case, the problem is that Quarto's build of bootstrap.min.css is taken from an older version of bslib, and does not have that CSS. As far as I know, there's not a way to cleanly override Quarto's version of Bootstrap.

What's additionally confusing is that these bootstrap.min.css files are not equivalent. It's not that Quarto takes an older bootstrap.min.css from bslib, but rather they took the base Bootstrap layers that contain some direct patches that bslib applies to the Bootstrap source. As a result, Quarto's version doesn't include any of the rules that might get layered on top in R via a Sass compilation step.

In Shiny for Python, the bootstrap.min.css is compiled via bslib and may include even more than bslib would include in "bootstrap" because in bslib and R we can compile the Sass on the fly, whereas in Python these rules need to ship as compiled.

At this point, I think it'd be worth the effort to put together a reprex.

@jcheng5
Copy link
Member

jcheng5 commented Oct 3, 2023

I'd really really like us to move away from this model (that I invented, IIRC?) where the sass rules are layered in, and instead have everything that builds on top of Bootstrap use CSS variables exclusively. How far do you think we are from that reality?

Until we do that, we'll never having theming in Shiny for Python, and Quarto will always act weird like this when we put our components in it.

@cpsievert
Copy link
Collaborator

How far do you think we are from that reality?

We're not that far off, and we've made significant strides recently with the bslib components.

instead have everything that builds on top of Bootstrap use CSS variables exclusively.

I doubt this alone would be sufficient for solving the core issue here, which as @wch pointed out, seems to be related to how Quarto is managing Bootstrap. After discussing with Charles today, we talked about some of these issues, and at least for now, I'll be sending them a PR so that Quarto can better simulate bslib, which may end up fixing this issue.

Longer term, it would be great to eliminate Quarto's build-time step of copying over bslib's Sass code. I don't know if this is realistic, but I'm thinking Quarto could maybe do something like library(bslib); bs_theme() |> bs_bundle(user_sass) |> bs_theme_dependencies() via webR (and maybe, for py-shiny, could have a web service to do this?)

@gadenbuie
Copy link
Member

gadenbuie commented Oct 4, 2023

After researching how py-shiny handles the ionRangSlider dependency, I have a much clearer understanding of this issue. py-shiny pre-renders the static ionRangeSlider.css using the preset = "shiny" theme (see here). That compiled CSS file isn't fully stand-alone, as we have additional rules in the shiny preset that end up in bootstrap.min.css.

So one possibility, for py-shiny at least, would be to include those rules in the pre-compiled ionRangeSlider.css file. Alternatively, it might be better for py-shiny to serve the standard Bootstrap styles for the ionRangeSlider when in a Quarto doc or when paired with an unknown Bootstrap.

Unfortunately, CSS variables wouldn't completely save us from this problem (even though I agree we should move in that direction with our existing components too), because we're missing an entire small stylesheet.

I do think we should at least open a py-shiny specific issue since the dynamics of the situation are different in Python and R and will need different fixes (unless we decide to make Bootstrap 5 and the Shiny preset the de facto default Shiny app look).

@wch
Copy link
Collaborator Author

wch commented Oct 4, 2023

Closing in favor of posit-dev/py-shiny#748

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

No branches or pull requests

4 participants