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

add fill_container argument for a responsive spinner ui container #77

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kalganem
Copy link

@kalganem kalganem commented Feb 2, 2023

Hi Dean,

Related to #76, I investigated the issue and it turns out that htmltools 0.5.4 added some new css classes that "are allowed to grow and shrink when their parent is opinionated about their height". The two main classes are "html-fill-item" and "html-fill-container".

So, I added a boolean argument to the withSpinner function that appends these css classes to the spinner container.
Obviously this requires to import the more recent htmltools update (>= 0.5.4).

Screenshot 2023-02-01 at 5 54 35 PM
Screenshot 2023-02-01 at 5 54 51 PM

Reprex:

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

ui <- page_fluid(
  card(
    full_screen = T,
    card_header("fill_container = F"),
    card_body_fill(withSpinner(plotlyOutput("plot1"), fill_container = F))
  ),
  card(
    full_screen = T,
    card_header("fill_container = T"),
    card_body_fill(withSpinner(plotlyOutput("plot2"), fill_container = T))
  ),
)

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)

Let me know if this is a good addition to the shinycssloaders package.

Also, this might help get rid of the proxy.height argument since the spinner will always fit the children? But I'm not sure about that since I haven't done enough with the proxy.height argument

Best,
Khaled

@daattali
Copy link
Owner

daattali commented Mar 5, 2023

Thanks for looking into this and taking the initiative to find the problem and possible fix!

I took a while to respond because I don't know how I feel about this. Something about this just doesn't feel clean. That's not your fault - I think htmltools/bslib in general is getting a little bloated and causing its too many dependencies to be very tightly coupled with it. It looks like the required CSS (html-fill-item) only gets included on the page if the function shiny::bindFillRole() gets called. This means that this new fill_container parameter is strongly dependent on shiny::bindFillRole(). It also means that it would only work if someone has the latest version of shiny installed, which is a pretty strict requirement.

In general, the loader does automatically try to take up the space of its parent (you can see that with shinycssloaders::withSpinner(plotOutput("plot", height = 900))). So I think this new parameter will only be helpful when using the new card_body_fill(), is that right?

@ncullen93
Copy link

+1 for this. The spinners are otherwise unusable with bslib cards. Unfortunately the work around that I had just stopped working with some htmltools update I did I guess...

DT::DTOutput('x') %>% shinycssloaders::withSpinner() %>% htmltools::tagAppendAttributes(class='html-fill-item html-fill-container')

That doesn't even work any more :(

@Aariq
Copy link

Aariq commented Oct 3, 2023

It would be nice if the default behavior of an output using a spinner was just the same as the output without the spinner—i.e., no additional fill_container argument needed. Is there a reason the shiny-spinner-output-container shouldn't always be fillable? I at least think the default for fill_container should probably be TRUE.

@daattali
Copy link
Owner

daattali commented Oct 13, 2023

I appreciate all the input here, but I think I'm still going to wait a little longer before dealing with this. I feel it's still too early to try to adhere to bslib's new features simply because bslib and htmltools are going through so many changes at the moment and bslib hasn't stabilized yet. bslib seems to be having breaking changes in almost every release, and it looks like the UI layout functionality is still trying to figure itself out. I'll revisit this when bslib and its counterparts (htmltools/shiny) seem more stable.

@ncullen93
Copy link

Totally agree. It has become much bigger than its original scope. Hoping it becomes more stable because bslib is one of the only consistently maintained package that is working to improve the state of high-level shiny UI components.

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

Successfully merging this pull request may close these issues.

4 participants