search

rstudio / bslib

Tools for theming Shiny and R Markdown via Bootstrap 3, 4, or 5.
https://rstudio.github.io/bslib/
Other
443 stars 50 forks source link

More `update_popover()` fixes #1017

Closed cpsievert closed 3 months ago

cpsievert commented 3 months ago

During investigation of #1011, I realized that particular issue can/should be addressed in the testing code, but also that update_popover() had a few other subtle issues:

  1. update_popover(title = character(0)) currently doesn't clear the current title (because no header gets passed from server to client).
  2. getOrCreateElement() was always taking fallback before selector as long as fallback is truthy. The implicit assumption here is that when the popover is visible, then fallback is falsy. I think it's better to just be explicit that if the popover is visible, we get the content from Bootstrap's .popover element. Otherwise, it comes from the children of our custom element (i.e., fallback).
  3. getOrCreateElement() also wasn't correctly taking the contents of the body/header when visible. In that case, it needs to be taking the children of .popover-body/.popover-title
  4. When updating just the title, it was possible to end up with 2 close buttons since we weren't accounting for the fact that "current content" might already have close button. So, now, we only add the close button when the content is being updated.

Testing notes

I've updated 316-bslib-popovers to have some better coverage of this code path in https://github.com/rstudio/shinycoreci/pull/263. That PR should get merged when this one gets merged.