daattali / shinycssloaders

⌛ Add loading animations to a Shiny output while it's recalculating
https://daattali.com/shiny/shinycssloaders-demo/
Other
395 stars 45 forks source link

withSpinner + bslib card_body_fill #76

Open kalganem opened 1 year ago

kalganem commented 1 year ago

Hi Dean,

Thanks for developing this awesome package!

I'm using the bslib's card function with the full_screen set as True which expands the card to fit viewport size. Plotly outputs are responsive and auto fit the large viewport, however when withSpinner is wrapped around plotly outputs, it prevents the auto fit behavior.

Screenshot 2023-02-01 at 2 41 21 PM Screenshot 2023-02-01 at 2 41 31 PM

Here's the reprex:

library(shiny)
library(shinycssloaders)
library(plotly)
library(bslib)

ui <- page_fluid(
  card(
    full_screen = T,
    card_header("without spinner"),
    card_body_fill(plotlyOutput("plot1"))
  ),
  card(
    full_screen = T,
    card_header("with spinner"),
    card_body_fill(withSpinner(plotlyOutput("plot2")))
  ),
)

server <- function(input, output) {

  output$plot1 <- renderPlotly({
    plot_ly(mtcars, x = ~mpg, y = ~cyl) |> add_markers()
  })

  output$plot2 <- renderPlotly({
    plot_ly(mtcars, x = ~mpg, y = ~cyl) |> add_markers()
  })

}

shinyApp(ui, server)

Is there a workaround around this issue?

For reference, here's the recent plolty update that made this work.

Thanks!

daattali commented 1 year ago

I can see the issue you describe. Thanks for reporting, and also for linking to plotly's change.

I'm afraid it's going to be very difficult for me to know how to accommodate this new functionality. Plotly's authors have the advantage of being the same author of {bslib} so they can make them work in tandem and know exactly what a change in one means for the other. We don't have this knowledge :)

By applying withSpinner(), the output gets wrapped in an additional <div>, so perhaps this card_body_fill is not happy with that. That's my only guess at the moment. I'd be happy to receive some help from anyone who has time looking into this and finding out the cause/solution. I don't know if I'll be ablet to address this any time soon, but hopefully someone can pick it up.

daattali commented 1 year ago

It looks like DT also had to make an update to allow a new parameter in order to support this https://github.com/rstudio/DT/releases/tag/v0.27 . I'm still of the opinion that it's not great practice for shiny to make changes that require their dependencies to accommodate... but I am starting to see that this specific feature is being adopted by a lot of RStudio's packages. So if you can't fight them, join them!

I'll review the PR next time I get around to this package