-
Notifications
You must be signed in to change notification settings - Fork 58
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
Conversation
The card might be inside more dynamic UI, so we can't just observe from the parent node.
@@ -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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
/** | ||
* 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(); | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Fixes #1003
Adds a new
ShinyRemovedObserver
class that's conceptually similar toShinyResizeObserver
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 theCard
class that watches thecard's parent elementthedocument.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 arenderUI()
(this is pretty common). TheShinyRemovedObserver
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 theCard
class is easier to read and understand without the boilerplate ofMutationObserver
.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.