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.

Sign up for GitHub

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

Merged
merged 5 commits into from
Mar 19, 2024
Merged

More update_popover() fixes #1017

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6d80df
Fix several issues with popover updating
cpsievert Mar 18, 2024
907646d
`devtools::document()` (GitHub Actions)
cpsievert Mar 18, 2024
d0194b7
Update R/popover.R
cpsievert Mar 18, 2024
6f1e085
`devtools::document()` (GitHub Actions)
cpsievert Mar 18, 2024
65e71f2
Better comment
cpsievert Mar 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions R/popover.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@
#' object in a [div()] or [span()].
#' @param ... UI elements for the popover's body. Character strings are
#' [automatically escaped][htmlEscape()] unless marked as [HTML()].
#' @param title A title (header) for the popover.
#' @param title A title (header) for the popover. To remove a header
#' with `update_popover()`, provide a either an empty string or `character(0)`.
#' @param id A character string. Required to re-actively respond to the
#' visibility of the popover (via the `input[[id]]` value) and/or update the
#' visibility/contents of the popover.
Expand Down Expand Up @@ -183,7 +184,7 @@ update_popover <- function(id, ..., title = NULL, session = get_current_session(
msg <- dropNulls(list(
method = "update",
content = if (length(body) > 0) processDeps(body, session),
header = if (length(title) > 0) processDeps(title, session)
header = if (!is.null(title)) processDeps(title, session)
))

force(id)
Expand Down
31 changes: 11 additions & 20 deletions inst/components/dist/web-components.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions inst/components/dist/web-components.js.map
View file
Open in desktop

Large diffs are not rendered by default.

14 changes: 7 additions & 7 deletions inst/components/dist/web-components.min.js
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 3 additions & 3 deletions inst/components/dist/web-components.min.js.map
View file
Open in desktop

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion man/popover.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

60 changes: 35 additions & 25 deletions srcts/src/components/webcomponents/popover.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,43 +349,53 @@ export class BslibPopover extends BslibElement {
private _updatePopover(data: UpdateMessage): void {
const { content, header } = data;

// First, render any HTMLDependency()s
const deps = [];
if (content) deps.push(...content.deps);
if (header) deps.push(...header.deps);
Shiny.renderDependencies(deps);

// Since we may need to add a close button (with event handlers),
// parse the string into an HTMLElement. And, since .setContent() is less
// error-prone if we pass _both_ the header and content, we fallback to the
// existing header/content if the user didn't supply one.
const getOrCreateElement = (
x: typeof content | typeof header,
fallback: HTMLElement | undefined,
selector: string
): HTMLElement => {
if (x) return createWrapperElement(x.html, "contents");
if (fallback) return fallback;
return this.bsPopover.tip?.querySelector(selector) as HTMLElement;
};
const { tip } = this.bsPopover;

const newHeader = getOrCreateElement(
header,
this.header,
".popover-header"
);
const newContent = getOrCreateElement(
content,
this.content,
".popover-body"
);
render(this._closeButton(newHeader), newContent);
// If the user hasn't supplied new content/header, we still need to find and pass
// along the current content/header (so they don't get removed on update).
// 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);
Copy link
Member

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?

Suggested change
// 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);

Copy link
Collaborator Author

@cpsievert cpsievert Mar 19, 2024 •
edited
Loading

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).


const currentContent = this.visible
? (tip?.querySelector(".popover-body")?.children[0] as HTMLElement)
: (this.content as HTMLElement);

// If user does supply new content/header, then we wrap it in a
// <div style="display:contents"> so that the markup structure mirrors
// what bslib::popover() uses for markup
const newHeader = header
? createWrapperElement(header.html, "contents")
: currentHeader;

const newContent = content
? createWrapperElement(content.html, "contents")
: currentContent;

// If user has supplied new content, then add the close button
// (if not, it's already there since we added it in connectedCallback)
if (content) {
render(this._closeButton(newHeader), newContent);
}

// Only render a header if the newHeader has actual contents
// (i.e., if header.html is an empty string, then we don't render it)
const actualHeader = hasHeader(newHeader) ? newHeader : "";

setContentCarefully({
instance: this.bsPopover,
trigger: this.triggerElement,
content: {
// eslint-disable-next-line @typescript-eslint/naming-convention
".popover-header": hasHeader(newHeader) ? newHeader : "",
".popover-header": actualHeader,
// eslint-disable-next-line @typescript-eslint/naming-convention
".popover-body": newContent,
},
Expand Down