-
Notifications
You must be signed in to change notification settings - Fork 22
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
#5078: support showing panel as popover #5023
Changes from 2 commits
977b33f
0341f6e
21ebc08
5463b7a
5bd588e
873e34f
55aadeb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -29,13 +29,17 @@ import { | |
import { type PanelPayload } from "@/sidebar/types"; | ||
import { | ||
stopWaitingForTemporaryPanels, | ||
cancelTemporaryPanelsForExtension, | ||
cancelTemporaryPanels, | ||
waitForTemporaryPanel, | ||
} from "@/blocks/transformers/temporaryInfo/temporaryPanelProtocol"; | ||
import { CancelError, PropError } from "@/errors/businessErrors"; | ||
import { BusinessError, CancelError, PropError } from "@/errors/businessErrors"; | ||
import { getThisFrame } from "webext-messenger"; | ||
import { showModal } from "@/blocks/transformers/ephemeralForm/modalUtils"; | ||
import { IS_ROOT_AWARE_BRICK_PROPS } from "@/blocks/rootModeHelpers"; | ||
import { showPopover } from "@/blocks/transformers/temporaryInfo/popoverUtils"; | ||
|
||
type Location = "panel" | "modal"; | ||
type Location = "panel" | "modal" | "popover"; | ||
|
||
export async function createFrameSource( | ||
nonce: string, | ||
|
@@ -62,6 +66,10 @@ class DisplayTemporaryInfo extends Transformer { | |
); | ||
} | ||
|
||
override async isRootAware(): Promise<boolean> { | ||
return true; | ||
} | ||
|
||
inputSchema: Schema = { | ||
type: "object", | ||
properties: { | ||
|
@@ -76,10 +84,11 @@ class DisplayTemporaryInfo extends Transformer { | |
}, | ||
location: { | ||
type: "string", | ||
enum: ["panel", "modal"], | ||
enum: ["panel", "modal", "popover"], | ||
default: "panel", | ||
description: "The location of the information (default='panel')", | ||
}, | ||
...IS_ROOT_AWARE_BRICK_PROPS, | ||
}, | ||
required: ["body"], | ||
}; | ||
|
@@ -89,65 +98,101 @@ class DisplayTemporaryInfo extends Transformer { | |
title, | ||
body: bodyPipeline, | ||
location = "panel", | ||
isRootAware = false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Safe default There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is the brick There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See this PR for more details on the approach: #4910 The idea is that:
|
||
}: BlockArg<{ | ||
title: string; | ||
location: Location; | ||
body: PipelineExpression; | ||
isRootAware: boolean; | ||
}>, | ||
{ | ||
logger: { | ||
context: { extensionId }, | ||
}, | ||
root, | ||
ctxt, | ||
runPipeline, | ||
runRendererPipeline, | ||
}: BlockOptions | ||
): Promise<UnknownObject | null> { | ||
expectContext("contentScript"); | ||
|
||
const target = isRootAware ? root : document; | ||
|
||
const nonce = uuidv4(); | ||
const controller = new AbortController(); | ||
|
||
const payload = (await runRendererPipeline(bodyPipeline?.__value__ ?? [], { | ||
key: "body", | ||
counter: 0, | ||
})) as PanelPayload; | ||
|
||
if (location === "panel") { | ||
await ensureSidebar(); | ||
const payload = (await runRendererPipeline( | ||
bodyPipeline?.__value__ ?? [], | ||
{ | ||
key: "body", | ||
counter: 0, | ||
}, | ||
{}, | ||
target | ||
)) as PanelPayload; | ||
|
||
switch (location) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Switch to a "switch" statement. The whitespace in the diff is bad |
||
case "panel": { | ||
await ensureSidebar(); | ||
|
||
showTemporarySidebarPanel({ | ||
extensionId, | ||
nonce, | ||
heading: title, | ||
payload, | ||
}); | ||
|
||
window.addEventListener( | ||
PANEL_HIDING_EVENT, | ||
() => { | ||
controller.abort(); | ||
}, | ||
{ | ||
signal: controller.signal, | ||
} | ||
); | ||
|
||
controller.signal.addEventListener("abort", () => { | ||
hideTemporarySidebarPanel(nonce); | ||
void stopWaitingForTemporaryPanels([nonce]); | ||
}); | ||
|
||
break; | ||
} | ||
|
||
showTemporarySidebarPanel({ | ||
extensionId, | ||
nonce, | ||
heading: title, | ||
payload, | ||
}); | ||
case "modal": { | ||
const frameSource = await createFrameSource(nonce, location); | ||
showModal(frameSource, controller); | ||
break; | ||
} | ||
|
||
window.addEventListener( | ||
PANEL_HIDING_EVENT, | ||
() => { | ||
controller.abort(); | ||
}, | ||
{ | ||
signal: controller.signal, | ||
case "popover": { | ||
const frameSource = await createFrameSource(nonce, location); | ||
if (target === document) { | ||
throw new BusinessError("Target must be an element for popover"); | ||
} | ||
); | ||
|
||
controller.signal.addEventListener("abort", () => { | ||
hideTemporarySidebarPanel(nonce); | ||
void stopWaitingForTemporaryPanels([nonce]); | ||
}); | ||
} else if (location === "modal") { | ||
const frameSource = await createFrameSource(nonce, location); | ||
showModal(frameSource, controller); | ||
} else { | ||
throw new PropError( | ||
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- dynamic check for validated value | ||
`Invalid location: ${location}`, | ||
this.id, | ||
"location", | ||
location | ||
); | ||
await cancelTemporaryPanelsForExtension(extensionId); | ||
|
||
const onHide = async () => { | ||
await cancelTemporaryPanels([nonce]); | ||
}; | ||
|
||
showPopover(frameSource, target as HTMLElement, onHide, controller); | ||
|
||
break; | ||
} | ||
|
||
default: { | ||
throw new PropError( | ||
// eslint-disable-next-line @typescript-eslint/restrict-template-expressions -- dynamic check for validated value | ||
`Invalid location: ${location}`, | ||
this.id, | ||
"location", | ||
location | ||
); | ||
} | ||
} | ||
|
||
let result = null; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,10 @@ | |
* along with this program. If not, see <http://www.gnu.org/licenses/>. | ||
*/ | ||
|
||
import cx from "classnames"; | ||
import React, { useEffect } from "react"; | ||
import { useAsyncState } from "@/hooks/common"; | ||
import { Modal } from "react-bootstrap"; | ||
import { Modal, Popover } from "react-bootstrap"; | ||
import { | ||
cancelTemporaryPanel, | ||
getPanelDefinition, | ||
|
@@ -31,9 +32,22 @@ import reportError from "@/telemetry/reportError"; | |
import ErrorBoundary from "@/components/ErrorBoundary"; | ||
import PanelBody from "@/sidebar/PanelBody"; | ||
|
||
const ModalLayout: React.FC = ({ children }) => ( | ||
type Mode = "modal" | "popover"; | ||
|
||
const ModalLayout: React.FC<{ className?: string }> = ({ | ||
className, | ||
children, | ||
}) => ( | ||
// Don't use React Bootstrap's Modal because we want to customize the classes in the layout | ||
<div className={cx("modal-content", className)}>{children}</div> | ||
); | ||
|
||
const PopoverLayout: React.FC<{ className?: string }> = ({ | ||
className, | ||
children, | ||
}) => ( | ||
// Don't use React Bootstrap's Modal because we want to customize the classes in the layout | ||
<div className="modal-content">{children}</div> | ||
<div className={cx("popover", className)}>{children}</div> | ||
); | ||
|
||
/** | ||
|
@@ -43,16 +57,25 @@ const EphemeralPanel: React.FC = () => { | |
const params = new URLSearchParams(location.search); | ||
const nonce = validateUUID(params.get("nonce")); | ||
const opener = JSON.parse(params.get("opener")) as Target; | ||
const mode = params.get("mode") as Mode; | ||
|
||
// The opener for a sidebar panel will be the sidebar frame, not the host panel frame. The sidebar only opens in the | ||
// top-level frame, so hard-code the top-level frameId | ||
const target = opener; | ||
|
||
const Layout = mode === "modal" ? ModalLayout : PopoverLayout; | ||
|
||
const [entry, isLoading, error] = useAsyncState( | ||
async () => getPanelDefinition(target, nonce), | ||
[nonce] | ||
); | ||
|
||
useEffect(() => { | ||
document.dispatchEvent( | ||
new CustomEvent("@@pixiebrix/PANEL_MOUNTED", { detail: nonce }) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should probably put this event name in a constant in |
||
); | ||
}, [nonce]); | ||
|
||
// Report error once | ||
useEffect(() => { | ||
if (error) { | ||
|
@@ -63,15 +86,15 @@ const EphemeralPanel: React.FC = () => { | |
|
||
if (isLoading) { | ||
return ( | ||
<ModalLayout> | ||
<Layout> | ||
<Loader /> | ||
</ModalLayout> | ||
</Layout> | ||
); | ||
} | ||
|
||
if (error) { | ||
return ( | ||
<ModalLayout> | ||
<Layout> | ||
<div>Panel Error</div> | ||
|
||
<div className="text-danger my-3">{getErrorMessage(error)}</div> | ||
|
@@ -87,20 +110,24 @@ const EphemeralPanel: React.FC = () => { | |
Close | ||
</button> | ||
</div> | ||
</ModalLayout> | ||
</Layout> | ||
); | ||
} | ||
|
||
return ( | ||
<ModalLayout> | ||
<Modal.Header | ||
closeButton | ||
onHide={() => { | ||
cancelTemporaryPanel(target, [nonce]); | ||
}} | ||
> | ||
<Modal.Title>{entry.heading}</Modal.Title> | ||
</Modal.Header> | ||
<Layout> | ||
{mode === "popover" ? ( | ||
<Popover.Title>{entry.heading}</Popover.Title> | ||
) : ( | ||
<Modal.Header | ||
closeButton | ||
onHide={() => { | ||
cancelTemporaryPanel(target, [nonce]); | ||
}} | ||
> | ||
<Modal.Title>{entry.heading}</Modal.Title> | ||
</Modal.Header> | ||
)} | ||
<Modal.Body> | ||
<ErrorBoundary> | ||
<PanelBody | ||
|
@@ -113,7 +140,7 @@ const EphemeralPanel: React.FC = () => { | |
/> | ||
</ErrorBoundary> | ||
</Modal.Body> | ||
</ModalLayout> | ||
</Layout> | ||
); | ||
}; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
import { createPopper } from "@popperjs/core"; | ||
|
||
// https://popper.js.org/docs/v2/performance/#attaching-elements-to-the-dom | ||
function ensureTooltipsContainer(): Element { | ||
let container = document.querySelector("#pb-tooltips-container"); | ||
if (!container) { | ||
container = document.createElement("div"); | ||
container.id = "pb-tooltips-container"; | ||
document.body.append(container); | ||
} | ||
|
||
return container; | ||
} | ||
|
||
export function showPopover( | ||
url: URL, | ||
element: HTMLElement, | ||
onHide: () => void, | ||
abortController: AbortController | ||
): void { | ||
const nonce = url.searchParams.get("nonce"); | ||
|
||
const $tooltip = $( | ||
`<div role="tooltip"><iframe src="${url.href}" title="Popover content" style="border: 0; flex-grow: 1; color-scheme: normal;" /></div>` | ||
); | ||
const tooltip: HTMLElement = $tooltip.get()[0]; | ||
|
||
ensureTooltipsContainer().append(tooltip); | ||
const $body = $(document.body); | ||
|
||
const popper = createPopper(element, tooltip, { | ||
placement: "auto", | ||
modifiers: [ | ||
{ | ||
name: "offset", | ||
options: { | ||
offset: [8, 8], | ||
}, | ||
}, | ||
], | ||
}); | ||
|
||
const updateListener = (event: Event) => { | ||
if (event instanceof CustomEvent && event.detail.nonce === nonce) { | ||
// Force popper resize | ||
void popper.update(); | ||
} | ||
}; | ||
|
||
document.addEventListener("@@pixiebrix/PANEL_MOUNTED", updateListener); | ||
|
||
const outsideClickListener = (event: JQuery.TriggeredEvent) => { | ||
if ($(event.target).closest(tooltip).length === 0) { | ||
onHide(); | ||
} | ||
}; | ||
|
||
// Hide tooltip on click outside | ||
$body.on("click touchend", outsideClickListener); | ||
|
||
abortController.signal.addEventListener("abort", () => { | ||
tooltip.remove(); | ||
popper.destroy(); | ||
document.removeEventListener("panelMounted", updateListener); | ||
$body.off("click touchend", outsideClickListener); | ||
}); | ||
} |
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.
Taking same approach as other PR for upgrading the brick to be root aware