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

Exit full-screen mode when a full-screen card is removed #1005

Merged
merged 15 commits into from Mar 15, 2024

Conversation

gadenbuie
Copy link
Member

@gadenbuie gadenbuie commented Mar 12, 2024

Fixes #1003

Adds a new ShinyRemovedObserver class that's conceptually similar to ShinyResizeObserver but instead calls a callback function on matching elements when they're removed from the DOM.

In this case, we instantiate a static cardRemovedObserver on the Card class that watches the card's parent element the document.body for changes and exits full screen mode when a full-screen card is removed.

Note that we can't watch the parent element because the card might be in a layout_columns() in a renderUI() (this is pretty common). The ShinyRemovedObserver is careful to do as little work as needed to find the matching elements, and I think the abstraction is still worth it. We might want to use it in other places and the callback in the Card class is easier to read and understand without the boilerplate of MutationObserver.

Example app

This example app contains a card that is dynamically rendered inside a renderUI(). When the card is modified in full screen mode, the currently-full-screen card is removed from the DOM and replaced with a new card (or not replaced at all, it's dynamic UI!).

Previously, we were not cleaning up the full screen state when a full-screen card was removed. Now, we see the old, full-screen card was removed and we call the exit full screen methods to reset the full screen state.

library(shiny)
library(bslib)

choices <- c("cyl", "disp")

ui <- page_fixed(
  uiOutput("content")
)

server <- function(input, output) {
  output$content <- renderUI({
    layout_columns(
      card(
        card_header("Marker:", input$marker),
        selectInput(
          "marker",
          "Marker:",
          choices = choices,
          selected = input$marker
        ),
        full_screen = TRUE,
        min_height = 300
      )
    )
  })
}

shinyApp(ui = ui, server = server)

@@ -93,6 +113,7 @@ class Card {
// Let Shiny know to trigger resize when the card size changes
// TODO: shiny could/should do this itself (rstudio/shiny#3682)
Card.shinyResizeObserver.observe(this.card);
Card.cardRemovedObserver.observe(document.body);
Copy link
Collaborator

@cpsievert cpsievert Mar 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The potential performance implications here makes me feel like it would really be worth doing #1009 first

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree in the sense that we should be suspicious of a MutationObserver applied to a root element like document.body. But I think you should look at the observer code, which does very little work, to assess the performance implications.

Comment on lines +64 to +75
/**
* Stops observing the specified element for removal.
* @param el The element to unobserve.
*/
unobserve(el: HTMLElement): void {
if (!this.watching.has(el)) return;
// MutationObserver doesn't have an "unobserve" method, so we have to
// disconnect and re-observe all elements that are still being watched.
this.watching.delete(el);
this._flush();
this._restartObserver();
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd prefer this to be commented out with a comment about how it currently isn't being used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not, I guess because blocks of commented code give me the ick. I'd prefer to delete than to comment out code, but I'd also prefer to have a complete implementation than a partial one. Maybe we run into a similar problem and can pick up this class and drop it in. Or maybe we delete this whole file in a few weeks.

@gadenbuie gadenbuie merged commit 43d46da into main Mar 15, 2024
12 of 13 checks passed
@gadenbuie gadenbuie deleted the card/fix-fullscreen-exit-by-removal branch March 15, 2024 15:32
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.

Clear full screen state when a full-screen card is removed from the page
2 participants