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.

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

Adding A Modal Component #1369

Open
wants to merge 23 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
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
28 changes: 14 additions & 14 deletions package-lock.json

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

8 changes: 4 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
"@surma/rollup-plugin-off-main-thread": "^2.2.2",
"@types/dedent": "^0.7.0",
"@types/mime-types": "^2.1.1",
"@types/node": "^16.11.1",
"@types/node": "^16.18.38",
"@web/rollup-plugin-import-meta-assets": "^1.0.6",
"comlink": "^4.3.0",
"cssnano": "^4.1.10",
Expand All @@ -45,9 +45,9 @@
"rollup": "^2.38.0",
"rollup-plugin-terser": "^7.0.2",
"serve": "^11.3.2",
"typescript": "^4.4.4",
"which": "^2.0.2",
"wasm-feature-detect": "^1.2.11"
"typescript": "^4.8.3",
"wasm-feature-detect": "^1.2.11",
"which": "^2.0.2"
},
"lint-staged": {
"*.{js,css,json,md,ts,tsx}": "prettier --write",
Expand Down
57 changes: 57 additions & 0 deletions src/client/lazy-app/Modal/ModalHint/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { h, Component, VNode } from 'preact';
import Modal from '..';
import { InfoIcon } from 'client/lazy-app/icons';
import { linkRef } from 'shared/prerendered-app/util';

import * as style from './style.css';
import 'add-css:./style.css';

interface Props {
modalTitle: string;
text?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change this to "label" or something that better describes its usage.

}

interface State {}

export default class ModalHint extends Component<Props, State> {
private modalComponent?: Modal;

private onclick = (event: Event) => {
if (!this.modalComponent)
throw new Error('ModalHint is missing a modalComponent');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can probably be skipped, as it's guaranteed to be there.


// Stop bubbled events from triggering the modal
if (!(event.currentTarget as Element).matches('button')) return;

this.modalComponent.showModal();
};

render({ modalTitle, text }: Props) {
return (
<span
class={style.modalHint}
onClick={(event) => {
// When the button is clicked, the event starts bubbling up
// which might cause unexpected behaviour
event.preventDefault();
event.stopImmediatePropagation();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think stopImmediatePropagation is needed.

}}
>
<button
class={style.modalButton}
onClick={this.onclick}
title="Learn more"
>
<InfoIcon></InfoIcon>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<InfoIcon></InfoIcon>
<InfoIcon/>

{text}
</button>
<Modal
ref={linkRef(this, 'modalComponent')}
icon={<InfoIcon></InfoIcon>}
title={modalTitle}
content={this.props.children as any}
></Modal>
</span>
);
}
}
31 changes: 31 additions & 0 deletions src/client/lazy-app/Modal/ModalHint/style.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
.modal-hint {
display: inline-block;
vertical-align: bottom;
}

.modal-button {
composes: unbutton from global;

color: inherit;

display: flex;
align-items: center;
gap: 0.5rem;

border-radius: 2px;
}

.modal-button:hover {
text-decoration: underline solid 1px currentColor;
text-underline-offset: 0.1em;
}

.modal-button:focus {
outline: white solid 1px;
outline-offset: 0.25em;
}

.modal-button > svg {
width: 1em;
height: 1em;
}
120 changes: 120 additions & 0 deletions src/client/lazy-app/Modal/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
import { h, Component, VNode, Fragment } from 'preact';
import * as style from './style.css';
import 'add-css:./style.css';
import { linkRef } from 'shared/prerendered-app/util';
import { cleanSet } from '../util/clean-modify';
import { animateFrom, animateTo } from '../util';
import { RoundedCrossIcon } from '../icons';

interface Props {
icon: VNode;
title: string;
content: VNode;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we decided that the content of the model should be the children of the modal?

}

interface State {}

export default class Modal extends Component<Props, State> {
private dialogElement?: HTMLDialogElement;

componentDidMount() {
if (!this.dialogElement) throw new Error('Modal missing');
}

private _closeOnTransitionEnd = () => {
// If modal does not exist
if (!this.dialogElement) return;

this.dialogElement.close();
};

showModal() {
if (!this.dialogElement) throw Error('Modal missing');
if (this.dialogElement.open) return;

this.dialogElement.showModal();
// animate modal opening
animateTo(
this.dialogElement,
[
{ opacity: 0, transform: 'translateY(50px)' },
{ opacity: 1, transform: 'translateY(0px)' },
],
{ duration: 250, easing: 'ease' },
);
// animate modal::backdrop
// some browsers don't support ::backdrop, catch those errors
try {
animateFrom(
this.dialogElement,
{ opacity: 0 },
{
duration: 250,
easing: 'ease',
pseudoElement: '::backdrop',
},
);
} catch (e) {}
}

private _hideModal() {
if (!this.dialogElement) throw Error('Modal missing / hidden');
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to assume dialogElement is there, since this component never presents a state where it isn't. Other instances of this can be skipped too.

if (!this.dialogElement.open) return;

// animate modal closing
const anim = animateTo(
this.dialogElement,
{ opacity: 0, transform: 'translateY(50px)' },
{ duration: 250, easing: 'ease' },
);
// animate modal::backdrop
// some browsers don't support ::backdrop, catch those errors
try {
animateTo(this.dialogElement, [{ opacity: 0 }], {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
animateTo(this.dialogElement, [{ opacity: 0 }], {
animateTo(this.dialogElement, { opacity: 0 }, {

duration: 250,
easing: 'ease',
pseudoElement: '::backdrop',
});
} catch (e) {}
anim.onfinish = this._closeOnTransitionEnd;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
anim.onfinish = this._closeOnTransitionEnd;
anim.finished.then(() => this._closeOnTransitionEnd());

Using the promise makes it clearer that we only expect this to happen once. Also, it avoids passing an event into a function that doesn't expect it. https://jakearchibald.com/2021/function-callback-risks/

}

private _onKeyDown = (event: KeyboardEvent) => {
// Default behaviour of <dialog> closes it instantly when you press Esc
// So we hijack it to smoothly hide the modal
if (event.key === 'Escape') {
this._hideModal();
event.preventDefault();
event.stopImmediatePropagation();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think stopImmediatePropagation is needed

}
};

render({ title, icon, content }: Props, {}: State) {
return (
<dialog
class={style['modalDialog']}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class={style['modalDialog']}
class={style.modalDialog}

ref={linkRef(this, 'dialogElement')}
onKeyDown={this._onKeyDown}
onClick={(event) => {
// Prevent clicks from leaking out of the dialog
event.preventDefault();
event.stopImmediatePropagation();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it needs to stop immediate propogation?

}}
>
<article class={style.article}>
<header class={style.header}>
<span class={style.modalIcon}>{icon}</span>
<span class={style.modalTitle}>{title}</span>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a heading element. <h1> seems fine.

<button class={style.closeButton} onClick={() => this._hideModal()}>
<RoundedCrossIcon></RoundedCrossIcon>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<RoundedCrossIcon></RoundedCrossIcon>
<RoundedCrossIcon/>

</button>
</header>
<div class={style.contentContainer}>
<article class={style.content}>{content}</article>
</div>
<footer class={style.footer}></footer>
</article>
</dialog>
);
}
}
Loading