-
Notifications
You must be signed in to change notification settings - Fork 60
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
More update_popover()
fixes
#1017
Conversation
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.
Nice! This code is a lot cleaner and easier to follow than the previous version. I just have one small suggestion to tweak a comment.
// These current elements should always be a <div style="display:contents"> | ||
// with the actual content inside. | ||
const currentHeader = this.visible | ||
? (tip?.querySelector(".popover-header")?.children[0] as HTMLElement) | ||
: (this.header as HTMLElement); |
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.
Do we query tip
because changes to the popover content while the popover is open might not be reflected in this.header
and this.content
?
I'm thinking about it in part because this line
current elements should always be a <div style="display:contents">
makes it seems like we might have a stable element to point to, in which case this.header
could hold the reference to that element directly.
Actually, after reading ahead a bit, I can see that the <div>
is probably replaced on update. Maybe just a wording tweak to the comment?
// These current elements should always be a <div style="display:contents"> | |
// with the actual content inside. | |
const currentHeader = this.visible | |
? (tip?.querySelector(".popover-header")?.children[0] as HTMLElement) | |
: (this.header as HTMLElement); | |
// We take the first child of header/content because we wrap both in a | |
// <div style="display:contents"> wrapper element below. | |
const currentHeader = this.visible | |
? (tip?.querySelector(".popover-header")?.children[0] as HTMLElement) | |
: (this.header as HTMLElement); |
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.
Do we query tip because changes to the popover content while the popover is open might not be reflected in this.header and this.content?
Yes, but also, when the tip is visible, this.header
and this.content
are actually empty because they move elsewhere in the DOM (specifically, from the custom element to the Bootstrap controlled .popover
element).
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:update_popover(title = character(0))
currently doesn't clear the current title (because noheader
gets passed from server to client).getOrCreateElement()
was always takingfallback
beforeselector
as long asfallback
is truthy. The implicit assumption here is that when the popover is visible, thenfallback
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
).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
Testing notes
I've updated
316-bslib-popovers
to have some better coverage of this code path in rstudio/shinycoreci#263. That PR should get merged when this one gets merged.