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

withSpinner + bslib card_body_fill #76

Open
kalganem opened this issue Feb 1, 2023 · 2 comments
Open

withSpinner + bslib card_body_fill #76

kalganem opened this issue Feb 1, 2023 · 2 comments

Comments

@kalganem
Copy link

kalganem commented Feb 1, 2023

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

daattali commented Feb 2, 2023

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants