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 13 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
56 changes: 56 additions & 0 deletions src/client/lazy-app/Modal/ModalHint/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
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;
content: VNode;
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, content }: Props) {
return (
<span
class={style.modalHint}
onClick={(event) => {
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.

}}
aryanpingle marked this conversation as resolved.
Show resolved Hide resolved
>
<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/>

{this.props.children}
</button>
<Modal
ref={linkRef(this, 'modalComponent')}
icon={<InfoIcon></InfoIcon>}
title={modalTitle}
content={content}
></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 @@
span.modal-hint {
aryanpingle marked this conversation as resolved.
Show resolved Hide resolved
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;
}
136 changes: 136 additions & 0 deletions src/client/lazy-app/Modal/index.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,136 @@
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 { animateTo } from '../util';

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 {
shown: boolean;
}

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

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

// Set inert by default
this.dialogElement.inert = true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you talk me through the usage of inert in this component?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a feeling it's another instance of obsolete code now that we're using the native dialog element, but here's the logic: when hidden, the dialog is made inert to prevent its contents from being TABbed to. Might be a remnant of the time when I didn't use the dialog element and had to ensure the modal wasn't focusable.

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 so. <dialog> content isn't tabbable when it's not-shown.


// Prevent events from leaking through the dialog
this.dialogElement.onclick = (event) => {
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.

Can you talk me through the reason for this, and why the event is attached this way, rather than the usual Preact way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No good reason, I'll refactor it

}

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

this.dialogElement.close();
this.dialogElement.inert = true;
this.setState({ shown: false });
};

showModal() {
if (!this.dialogElement || this.dialogElement.open)
throw Error('Modal missing / already shown');
Copy link
Collaborator

Choose a reason for hiding this comment

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

My preference is that, if the thing is already in the desired state, then just return rather than throwing. Also, try to avoid using both component state & dialog element state. Use one as the source of truth (the component state?)


this.dialogElement.inert = false;
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 {
animateTo(this.dialogElement, [{ opacity: 0 }, { opacity: 1 }], {
duration: 250,
easing: 'linear',
aryanpingle marked this conversation as resolved.
Show resolved Hide resolved
pseudoElement: '::backdrop',
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense to use animateFrom in cases like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(For browsers that support backdrop) We need fill-mode forwards or both to keep the backdrop opaque after the animation - but animateFrom uses back.
If I edit that part of animateFrom then it essentially becomes animateTo without committing styles, and in that case a try-catch is much more readable IMO.

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 I get it, but wouldn't the backdrop's default from-CSS opacity be 1, so it wouldn't need a fill-forward?

} catch (e) {}
this.setState({ shown: true });
}

private _hideModal() {
if (!this.dialogElement || !this.dialogElement.open)
throw Error('Modal missing / hidden');

// 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: 'linear',
aryanpingle marked this conversation as resolved.
Show resolved Hide resolved
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(e: KeyboardEvent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: Rename the arg to event. Leave minification to the minifier 😄

// Default behaviour of <dialog> closes it instantly when you press Esc
// So we hijack it to smoothly hide the modal
if (e.key === 'Escape') {
this._hideModal();
e.preventDefault();
e.stopImmediatePropagation();
}
}

render({ title, icon, content }: Props, { shown }: State) {
return (
<dialog
ref={linkRef(this, 'dialogElement')}
onKeyDown={(e) => this._onKeyDown(e)}
aryanpingle marked this conversation as resolved.
Show resolved Hide resolved
>
{shown && (
<Fragment>
<header class={style.header}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you're going to use <header> and <footer>, then wrap both in an <article>. This semantically links the header and footer.

<span class={style.modalIcon}>{icon}</span>
<span class={style.modalTitle}>{title}</span>
<button
class={style.closeButton}
onClick={() => this._hideModal()}
>
<svg viewBox="0 0 480 480" fill="currentColor">
<path
d="M119.356 120L361 361M360.644 120L119 361"
stroke="#fff"
stroke-width="37"
stroke-linecap="round"
/>
</svg>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move this to icons?

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