Skip to content

Commit

Permalink
Display notebook in a native dialog when possible
Browse files Browse the repository at this point in the history
  • Loading branch information
acelaya committed Feb 8, 2024
1 parent c19a5a2 commit 5f3dc9c
Show file tree
Hide file tree
Showing 4 changed files with 157 additions and 44 deletions.
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
"@babel/preset-react": "^7.0.0",
"@babel/preset-typescript": "^7.16.7",
"@hypothesis/frontend-build": "^3.0.0",
"@hypothesis/frontend-shared": "^7.2.0",
"@hypothesis/frontend-shared": "^7.3.0",
"@hypothesis/frontend-testing": "^1.2.0",
"@npmcli/arborist": "^7.0.0",
"@octokit/rest": "^20.0.1",
Expand Down
119 changes: 90 additions & 29 deletions src/annotator/components/NotebookModal.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
import { IconButton, CancelIcon } from '@hypothesis/frontend-shared';
import {
IconButton,
CancelIcon,
ModalDialog,
} from '@hypothesis/frontend-shared';
import classnames from 'classnames';
import { useEffect, useRef, useState } from 'preact/hooks';
import type { ComponentChildren } from 'preact';
import { useEffect, useMemo, useRef, useState } from 'preact/hooks';

import { addConfigFragment } from '../../shared/config-fragment';
import { createAppConfig } from '../config/app';
Expand All @@ -27,7 +32,7 @@ function NotebookIframe({ config, groupId }: NotebookIframeProps) {
const notebookAppSrc = addConfigFragment(config.notebookAppUrl, {
...createAppConfig(config.notebookAppUrl, config),

// Explicity set the "focused" group
// Explicitly set the "focused" group
group: groupId,
});

Expand All @@ -40,9 +45,62 @@ function NotebookIframe({ config, groupId }: NotebookIframeProps) {
/>
);
}

/**
* Checks if the browser supports native modal dialogs
*/
function isModalDialogSupported(document: Document) {
const dialog = document.createElement('dialog');
return typeof dialog.showModal === 'function';
}

export type NotebookModalProps = {
eventBus: EventBus;
config: NotebookConfig;

/** Test seam */
document_?: Document;
};

type WrapperProps = { isHidden: boolean; children: ComponentChildren };

const DialogWrapper = ({ isHidden, children }: WrapperProps) => {
const dialogRef = useRef<HTMLDialogElement | null>(null);

useEffect(() => {
if (isHidden) {
dialogRef.current?.close();
} else {
dialogRef.current?.showModal();
}
}, [isHidden]);

return (
<dialog
ref={dialogRef}
className="relative w-full h-full backdrop:bg-black/50"
data-testid="notebook-outer"
>
{children}
</dialog>
);
};

const LegacyWrapper = ({ isHidden, children }: WrapperProps) => {
return (
<div
className={classnames({ hidden: isHidden })}
data-testid="notebook-outer"
>
<ModalDialog
variant="custom"
size="custom"
classes="p-3 relative w-full h-full"
>
{children}
</ModalDialog>
</div>
);
};

/**
Expand All @@ -51,6 +109,8 @@ export type NotebookModalProps = {
export default function NotebookModal({
eventBus,
config,
/* istanbul ignore next - test seam */
document_ = document,
}: NotebookModalProps) {
// Temporary solution: while there is no mechanism to sync new annotations in
// the notebook, we force re-rendering of the iframe on every 'openNotebook'
Expand All @@ -62,6 +122,15 @@ export default function NotebookModal({
const originalDocumentOverflowStyle = useRef('');
const emitterRef = useRef<Emitter | null>(null);

const isDialogSupported = useMemo(
() => isModalDialogSupported(document_),
[document_],
);
const Wrapper = useMemo(
() => (isDialogSupported ? DialogWrapper : LegacyWrapper),
[isDialogSupported],
);

// Stores the original overflow CSS property of document.body and reset it
// when the component is destroyed
useEffect(() => {
Expand Down Expand Up @@ -106,32 +175,24 @@ export default function NotebookModal({
}

return (
<div
className={classnames(
'fixed z-max top-0 left-0 right-0 bottom-0 p-3 bg-black/50',
{ hidden: isHidden },
)}
data-testid="notebook-outer"
>
<div className="relative w-full h-full" data-testid="notebook-inner">
<div className="absolute right-0 m-3">
<IconButton
title="Close the Notebook"
onClick={onClose}
variant="dark"
classes={classnames(
// Remove the dark variant's background color to avoid
// interfering with modal overlays. Re-activate the dark variant's
// background color on hover.
// See https://github.com/hypothesis/client/issues/3676
'!bg-transparent enabled:hover:!bg-grey-3',
)}
>
<CancelIcon className="w-4 h-4" />
</IconButton>
</div>
<NotebookIframe key={iframeKey} config={config} groupId={groupId} />
<Wrapper isHidden={isHidden}>
<div className="absolute right-0 m-3">
<IconButton
title="Close the Notebook"
onClick={onClose}
variant="dark"
classes={classnames(
// Remove the dark variant's background color to avoid
// interfering with modal overlays. Re-activate the dark variant's
// background color on hover.
// See https://github.com/hypothesis/client/issues/3676
'!bg-transparent enabled:hover:!bg-grey-3',
)}
>
<CancelIcon className="w-4 h-4" />
</IconButton>
</div>
</div>
<NotebookIframe key={iframeKey} config={config} groupId={groupId} />
</Wrapper>
);
}
76 changes: 64 additions & 12 deletions src/annotator/components/test/NotebookModal-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,24 +9,32 @@ describe('NotebookModal', () => {
const notebookURL = 'https://test.hypothes.is/notebook';

let components;
let containers;
let eventBus;
let emitter;

const outerSelector = '[data-testid="notebook-outer"]';

const createComponent = config => {
const createComponent = (config, fakeDocument) => {
const attachTo = document.createElement('div');
document.body.appendChild(attachTo);
containers.push(attachTo);

const component = mount(
<NotebookModal
eventBus={eventBus}
config={{ notebookAppUrl: notebookURL, ...config }}
document_={fakeDocument}
/>,
{ attachTo },
);
components.push(component);
return component;
};

beforeEach(() => {
components = [];
containers = [];
eventBus = new EventBus();
emitter = eventBus.createEmitter();

Expand All @@ -43,6 +51,7 @@ describe('NotebookModal', () => {

afterEach(() => {
components.forEach(component => component.unmount());
containers.forEach(container => container.remove());
$imports.$restore();
});

Expand Down Expand Up @@ -114,23 +123,66 @@ describe('NotebookModal', () => {
assert.equal(document.body.style.overflow, 'hidden');
});

it('hides modal on closing', () => {
const wrapper = createComponent();
context('when modal dialog is not supported', () => {
let fakeDocument;

emitter.publish('openNotebook', 'myGroup');
wrapper.update();
beforeEach(() => {
fakeDocument = {
createElement: sinon.stub().returns({}),
};
});

let outer = wrapper.find(outerSelector);
assert.isFalse(outer.hasClass('hidden'));
it('does not render a dialog element', () => {
const wrapper = createComponent({}, fakeDocument);

act(() => {
wrapper.find('IconButton').prop('onClick')();
emitter.publish('openNotebook', 'myGroup');
wrapper.update();

assert.isFalse(wrapper.exists('dialog'));
});
wrapper.update();

outer = wrapper.find(outerSelector);
it('hides modal on closing', () => {
const wrapper = createComponent({}, fakeDocument);

emitter.publish('openNotebook', 'myGroup');
wrapper.update();

assert.isTrue(outer.hasClass('hidden'));
let outer = wrapper.find(outerSelector);
assert.isFalse(outer.hasClass('hidden'));

act(() => {
wrapper.find('IconButton').prop('onClick')();
});
wrapper.update();

outer = wrapper.find(outerSelector);

assert.isTrue(outer.hasClass('hidden'));
});
});

context('when modal dialog is supported', () => {
it('renders a dialog element', () => {
const wrapper = createComponent({});

emitter.publish('openNotebook', 'myGroup');
wrapper.update();

assert.isTrue(wrapper.exists('dialog'));
});

it('shows and closes native dialog', () => {
const wrapper = createComponent({});
const isDialogOpen = () => wrapper.find('dialog').getDOMNode().open;

act(() => emitter.publish('openNotebook', 'myGroup'));
wrapper.update();
assert.isTrue(isDialogOpen());

act(() => wrapper.find('IconButton').prop('onClick')());
wrapper.update();
assert.isFalse(isDialogOpen());
});
});

it('resets document scrollability on closing the modal', () => {
Expand Down
4 changes: 2 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2695,7 +2695,7 @@ __metadata:
languageName: node
linkType: hard

"@hypothesis/frontend-shared@npm:^7.2.0":
"@hypothesis/frontend-shared@npm:^7.3.0":
version: 7.3.0
resolution: "@hypothesis/frontend-shared@npm:7.3.0"
dependencies:
Expand Down Expand Up @@ -9040,7 +9040,7 @@ __metadata:
"@babel/preset-react": ^7.0.0
"@babel/preset-typescript": ^7.16.7
"@hypothesis/frontend-build": ^3.0.0
"@hypothesis/frontend-shared": ^7.2.0
"@hypothesis/frontend-shared": ^7.3.0
"@hypothesis/frontend-testing": ^1.2.0
"@npmcli/arborist": ^7.0.0
"@octokit/rest": ^20.0.1
Expand Down

0 comments on commit 5f3dc9c

Please sign in to comment.