From d531cbbc702b551bed498d6bdf014a4af65fe1fa Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Mon, 9 Aug 2021 16:51:48 +0200 Subject: [PATCH 01/11] Rewrite ModalManagerState into Typescript - Fixes `attrs` parameter being marked as required - Add `isModalOpen` method --- js/src/common/states/ModalManagerState.js | 46 -------------- js/src/common/states/ModalManagerState.ts | 75 +++++++++++++++++++++++ 2 files changed, 75 insertions(+), 46 deletions(-) delete mode 100644 js/src/common/states/ModalManagerState.js create mode 100644 js/src/common/states/ModalManagerState.ts diff --git a/js/src/common/states/ModalManagerState.js b/js/src/common/states/ModalManagerState.js deleted file mode 100644 index a11196279d..0000000000 --- a/js/src/common/states/ModalManagerState.js +++ /dev/null @@ -1,46 +0,0 @@ -import Modal from '../components/Modal'; - -export default class ModalManagerState { - constructor() { - this.modal = null; - } - - /** - * Show a modal dialog. - * - * @public - */ - show(componentClass, attrs) { - if (!(componentClass.prototype instanceof Modal)) { - // This is duplicated so that if the error is caught, an error message still shows up in the debug console. - const invalidModalWarning = 'The ModalManager can only show Modals.'; - console.error(invalidModalWarning); - throw new Error(invalidModalWarning); - } - - clearTimeout(this.closeTimeout); - - this.modal = { componentClass, attrs }; - - m.redraw.sync(); - } - - /** - * Close the modal dialog. - * - * @public - */ - close() { - if (!this.modal) return; - - // Don't hide the modal immediately, because if the consumer happens to call - // the `show` method straight after to show another modal dialog, it will - // cause Bootstrap's modal JS to misbehave. Instead we will wait for a tiny - // bit to give the `show` method the opportunity to prevent this from going - // ahead. - this.closeTimeout = setTimeout(() => { - this.modal = null; - m.redraw(); - }); - } -} diff --git a/js/src/common/states/ModalManagerState.ts b/js/src/common/states/ModalManagerState.ts new file mode 100644 index 0000000000..547238bc7e --- /dev/null +++ b/js/src/common/states/ModalManagerState.ts @@ -0,0 +1,75 @@ +import Modal from '../components/Modal'; + +/** + * Class used to manage modal state. + * + * Accessible on the `app` object via `app.modal` property. + */ +export default class ModalManagerState { + /** + * @internal + */ + modal: null | { + componentClass: typeof Modal; + attrs?: Record; + }; + + private closeTimeout?: number; + + constructor() { + this.modal = null; + } + + /** + * Shows a modal dialog. + * + * If a modal is already open, the existing one will close and the new modal will replace it. + * + * @example Show a modal + * app.modal.show(MyCoolModal, { attr: 'value' }); + * + * @example Show a modal from a lifecycle method (`oncreate`, `view`, etc.) + * // This "hack" is needed due to quirks with nested redraws in Mithril. + * setTimeout(() => app.modal.show(MyCoolModal, { attr: 'value' }), 0); + */ + show(componentClass: typeof Modal, attrs: Record = {}): void { + if (!(componentClass.prototype instanceof Modal)) { + // This is duplicated so that if the error is caught, an error message still shows up in the debug console. + const invalidModalWarning = 'The ModalManager can only show Modals.'; + console.error(invalidModalWarning); + throw new Error(invalidModalWarning); + } + + clearTimeout(this.closeTimeout); + + this.modal = { componentClass, attrs }; + + m.redraw.sync(); + } + + /** + * Closes the currently open dialog, if one is open. + */ + close(): void { + if (!this.modal) return; + + // Don't hide the modal immediately, because if the consumer happens to call + // the `show` method straight after to show another modal dialog, it will + // cause Bootstrap's modal JS to misbehave. Instead we will wait for a tiny + // bit to give the `show` method the opportunity to prevent this from going + // ahead. + this.closeTimeout = setTimeout(() => { + this.modal = null; + m.redraw(); + }); + } + + /** + * Checks if a modal is currently open. + * + * @returns `true` if a modal dialog is currently open, otherwise `false`. + */ + isModalOpen(): boolean { + return !!this.modal; + } +} From 9017aa780b90df3cb88f5cc86efe919c012c4914 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Mon, 9 Aug 2021 17:05:51 +0200 Subject: [PATCH 02/11] Rewrite ModalManager into Typescript --- .../{ModalManager.js => ModalManager.tsx} | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) rename js/src/common/components/{ModalManager.js => ModalManager.tsx} (61%) diff --git a/js/src/common/components/ModalManager.js b/js/src/common/components/ModalManager.tsx similarity index 61% rename from js/src/common/components/ModalManager.js rename to js/src/common/components/ModalManager.tsx index 9d0609c183..59a373c211 100644 --- a/js/src/common/components/ModalManager.js +++ b/js/src/common/components/ModalManager.tsx @@ -1,29 +1,35 @@ import Component from '../Component'; +import type Mithril from 'mithril'; +import type ModalManagerState from '../states/ModalManagerState'; + +interface IModalManagerAttrs { + state: ModalManagerState; +} + /** * The `ModalManager` component manages a modal dialog. Only one modal dialog * can be shown at once; loading a new component into the ModalManager will * overwrite the previous one. */ -export default class ModalManager extends Component { +export default class ModalManager extends Component { view() { const modal = this.attrs.state.modal; return (
- {modal - ? modal.componentClass.component({ - ...modal.attrs, - animateShow: this.animateShow.bind(this), - animateHide: this.animateHide.bind(this), - state: this.attrs.state, - }) - : ''} + {modal && + modal.componentClass.component({ + ...modal.attrs, + animateShow: this.animateShow.bind(this), + animateHide: this.animateHide.bind(this), + state: this.attrs.state, + })}
); } - oncreate(vnode) { + oncreate(vnode: Mithril.VnodeDOM) { super.oncreate(vnode); // Ensure the modal state is notified about a closed modal, even when the @@ -32,7 +38,9 @@ export default class ModalManager extends Component { this.$().on('hidden.bs.modal', this.attrs.state.close.bind(this.attrs.state)); } - animateShow(readyCallback) { + animateShow(readyCallback: () => void): void { + if (!this.attrs.state.modal) return; + const dismissible = !!this.attrs.state.modal.componentClass.isDismissible; // If we are opening this modal while another modal is already open, @@ -45,6 +53,7 @@ export default class ModalManager extends Component { this.$() .one('shown.bs.modal', readyCallback) + // @ts-expect-error: No typings available for Bootstrap modals. .modal({ backdrop: dismissible || 'static', keyboard: dismissible, @@ -52,7 +61,8 @@ export default class ModalManager extends Component { .modal('show'); } - animateHide() { + animateHide(): void { + // @ts-expect-error: No typings available for Bootstrap modals. this.$().modal('hide'); } } From 624e55b3a5da0284d197df3affe1df4564ed80a0 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 3 Sep 2021 20:42:57 +0100 Subject: [PATCH 03/11] Fix incorrect type --- js/src/common/utils/RequestError.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/js/src/common/utils/RequestError.ts b/js/src/common/utils/RequestError.ts index 73e36f8549..e9a62589c3 100644 --- a/js/src/common/utils/RequestError.ts +++ b/js/src/common/utils/RequestError.ts @@ -1,21 +1,21 @@ export default class RequestError { - status: string; - options: object; + status: number; + options: Record; xhr: XMLHttpRequest; responseText: string | null; - response: object | null; + response: Record | null; alert: any; - constructor(status: string, responseText: string | null, options: object, xhr: XMLHttpRequest) { + constructor(status: number, responseText: string | null, options: Record, xhr: XMLHttpRequest) { this.status = status; this.responseText = responseText; this.options = options; this.xhr = xhr; try { - this.response = JSON.parse(responseText); + this.response = JSON.parse(responseText ?? 'null'); } catch (e) { this.response = null; } From 2b58df4874961e570f8019bebb139abc4b7773f4 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 3 Sep 2021 20:43:07 +0100 Subject: [PATCH 04/11] Continue modal rewrite --- .../common/components/{Modal.js => Modal.tsx} | 80 +++++++++---------- js/src/common/states/ModalManagerState.ts | 6 +- 2 files changed, 41 insertions(+), 45 deletions(-) rename js/src/common/components/{Modal.js => Modal.tsx} (58%) diff --git a/js/src/common/components/Modal.js b/js/src/common/components/Modal.tsx similarity index 58% rename from js/src/common/components/Modal.js rename to js/src/common/components/Modal.tsx index 677399ba43..61f52b79b4 100644 --- a/js/src/common/components/Modal.js +++ b/js/src/common/components/Modal.tsx @@ -1,33 +1,46 @@ +import Mithril from 'mithril'; import Component from '../Component'; +import type ModalManagerState from '../states/ModalManagerState'; +import RequestError from '../utils/RequestError'; import Alert from './Alert'; import Button from './Button'; +import type ModalManager from './ModalManager'; + +interface IRealModalAttrs { + state: ModalManagerState; + animateShow: ModalManager['animateShow']; + animateHide: ModalManager['animateHide']; +} + +export interface IModalAttrs { + dismissible: boolean; + [key: string]: unknown; +} /** * The `Modal` component displays a modal dialog, wrapped in a form. Subclasses * should implement the `className`, `title`, and `content` methods. - * - * @abstract */ -export default class Modal extends Component { +export default abstract class Modal extends Component { /** * Determine whether or not the modal should be dismissible via an 'x' button. */ - static isDismissible = true; + static readonly isDismissible = true; + + protected loading: boolean = false; /** * Attributes for an alert component to show below the header. - * - * @type {object} */ - alertAttrs = null; + alertAttrs!: ModalAttrs; - oncreate(vnode) { + oncreate(vnode: Mithril.VnodeDOM) { super.oncreate(vnode); - this.attrs.animateShow(() => this.onready()); + this.attrs.animateShow(this.onready); } - onbeforeremove(vnode) { + onbeforeremove(vnode: Mithril.VnodeDOM): Promise | void { super.onbeforeremove(vnode); // If the global modal state currently contains a modal, @@ -50,7 +63,7 @@ export default class Modal extends Component { return (
- {this.constructor.isDismissible ? ( + {(this.constructor as any).isDismissible && (
{Button.component({ icon: 'fas fa-times', @@ -58,8 +71,6 @@ export default class Modal extends Component { className: 'Button Button--icon Button--link', })}
- ) : ( - '' )}
@@ -78,51 +89,42 @@ export default class Modal extends Component { /** * Get the class name to apply to the modal. - * - * @return {String} - * @abstract */ - className() {} + abstract className(): string; /** * Get the title of the modal dialog. - * - * @return {String} - * @abstract */ - title() {} + abstract title(): string; /** * Get the content of the modal. - * - * @return {VirtualElement} - * @abstract */ - content() {} + abstract content(): Mithril.Children; /** * Handle the modal form's submit event. - * - * @param {Event} e */ - onsubmit() {} + abstract onsubmit(e: Event): void; /** - * Focus on the first input when the modal is ready to be used. + * Callback executed when the modal is shown and ready to be interacted with. + * + * @remark Focuses the first input in the modal. */ - onready() { - this.$('form').find('input, select, textarea').first().focus().select(); + onready(): void { + this.$().find('input, select, textarea').first().trigger('focus').trigger('select'); } /** - * Hide the modal. + * Hides the modal. */ hide() { this.attrs.state.close(); } /** - * Stop loading. + * Sets `loading` to false and triggers a redraw. */ loaded() { this.loading = false; @@ -130,18 +132,16 @@ export default class Modal extends Component { } /** - * Show an alert describing an error returned from the API, and give focus to - * the first relevant field. - * - * @param {RequestError} error + * Shows an alert describing an error returned from the API, and gives focus to + * the first relevant field involved in the error. */ - onerror(error) { + onerror(error: RequestError) { this.alertAttrs = error.alert; m.redraw(); - if (error.status === 422 && error.response.errors) { - this.$('form [name=' + error.response.errors[0].source.pointer.replace('/data/attributes/', '') + ']').select(); + if (error.status === 422 && error.response?.errors) { + this.$('form [name=' + (error.response.errors as any[])[0].source.pointer.replace('/data/attributes/', '') + ']').trigger('select'); } else { this.onready(); } diff --git a/js/src/common/states/ModalManagerState.ts b/js/src/common/states/ModalManagerState.ts index 547238bc7e..b01bc15e29 100644 --- a/js/src/common/states/ModalManagerState.ts +++ b/js/src/common/states/ModalManagerState.ts @@ -12,14 +12,10 @@ export default class ModalManagerState { modal: null | { componentClass: typeof Modal; attrs?: Record; - }; + } = null; private closeTimeout?: number; - constructor() { - this.modal = null; - } - /** * Shows a modal dialog. * From 5ce4a1d6722d695e8de488ebe6c2fa622f9eab81 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 3 Sep 2021 20:55:59 +0100 Subject: [PATCH 05/11] Update attr typings --- js/src/common/components/Modal.tsx | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/js/src/common/components/Modal.tsx b/js/src/common/components/Modal.tsx index 61f52b79b4..54c80e3a62 100644 --- a/js/src/common/components/Modal.tsx +++ b/js/src/common/components/Modal.tsx @@ -1,27 +1,23 @@ -import Mithril from 'mithril'; import Component from '../Component'; -import type ModalManagerState from '../states/ModalManagerState'; -import RequestError from '../utils/RequestError'; -import Alert from './Alert'; +import Alert, { AlertAttrs } from './Alert'; import Button from './Button'; + +import type Mithril from 'mithril'; +import type ModalManagerState from '../states/ModalManagerState'; +import type RequestError from '../utils/RequestError'; import type ModalManager from './ModalManager'; -interface IRealModalAttrs { +interface IInternalModalAttrs { state: ModalManagerState; animateShow: ModalManager['animateShow']; animateHide: ModalManager['animateHide']; } -export interface IModalAttrs { - dismissible: boolean; - [key: string]: unknown; -} - /** * The `Modal` component displays a modal dialog, wrapped in a form. Subclasses * should implement the `className`, `title`, and `content` methods. */ -export default abstract class Modal extends Component { +export default abstract class Modal = {}> extends Component { /** * Determine whether or not the modal should be dismissible via an 'x' button. */ @@ -32,15 +28,15 @@ export default abstract class Modal extends Comp /** * Attributes for an alert component to show below the header. */ - alertAttrs!: ModalAttrs; + alertAttrs!: AlertAttrs; - oncreate(vnode: Mithril.VnodeDOM) { + oncreate(vnode: Mithril.VnodeDOM) { super.oncreate(vnode); this.attrs.animateShow(this.onready); } - onbeforeremove(vnode: Mithril.VnodeDOM): Promise | void { + onbeforeremove(vnode: Mithril.VnodeDOM): Promise | void { super.onbeforeremove(vnode); // If the global modal state currently contains a modal, From 4511d7d522d278086ab823e1499971240fbbf659 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Fri, 10 Sep 2021 21:10:14 +0100 Subject: [PATCH 06/11] Fix correctly cast `this.constructor` calls --- js/src/common/Component.ts | 4 ++-- js/src/common/components/Modal.tsx | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/js/src/common/Component.ts b/js/src/common/Component.ts index 09b91b0197..3659eae665 100644 --- a/js/src/common/Component.ts +++ b/js/src/common/Component.ts @@ -138,13 +138,13 @@ export default abstract class Component = return (
- {(this.constructor as any).isDismissible && ( + {(this.constructor as typeof Modal).isDismissible && (
{Button.component({ icon: 'fas fa-times', From 7b4f859c78d240a3b64939a12a753de59bbe75c1 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Tue, 21 Sep 2021 00:33:55 +0100 Subject: [PATCH 07/11] Cast to bool --- js/src/common/components/ModalManager.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index 59a373c211..a9d55a1243 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -18,7 +18,7 @@ export default class ModalManager extends Component { return (
- {modal && + {!!modal && modal.componentClass.component({ ...modal.attrs, animateShow: this.animateShow.bind(this), From 70dbd0db4acda110767b468c8c032fa7f2a446f5 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Tue, 21 Sep 2021 09:26:47 +0100 Subject: [PATCH 08/11] Don't extend ModalAttrs by Record --- js/src/common/components/Modal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/common/components/Modal.tsx b/js/src/common/components/Modal.tsx index 7f54cbf722..e97169e5e1 100644 --- a/js/src/common/components/Modal.tsx +++ b/js/src/common/components/Modal.tsx @@ -17,7 +17,7 @@ interface IInternalModalAttrs { * The `Modal` component displays a modal dialog, wrapped in a form. Subclasses * should implement the `className`, `title`, and `content` methods. */ -export default abstract class Modal = {}> extends Component { +export default abstract class Modal extends Component { /** * Determine whether or not the modal should be dismissible via an 'x' button. */ From 17960bd2d6342c96840d24335cbb4920a6422021 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Tue, 21 Sep 2021 09:56:53 +0100 Subject: [PATCH 09/11] Prevent missing abstract methods in child Modals from bricking frontend --- js/src/common/components/Modal.tsx | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/js/src/common/components/Modal.tsx b/js/src/common/components/Modal.tsx index e97169e5e1..b5cc33f83d 100644 --- a/js/src/common/components/Modal.tsx +++ b/js/src/common/components/Modal.tsx @@ -6,6 +6,7 @@ import type Mithril from 'mithril'; import type ModalManagerState from '../states/ModalManagerState'; import type RequestError from '../utils/RequestError'; import type ModalManager from './ModalManager'; +import fireDebugWarning from '../helpers/fireDebugWarning'; interface IInternalModalAttrs { state: ModalManagerState; @@ -30,6 +31,31 @@ export default abstract class Modal extends Component) { + super.oninit(vnode); + + // TODO: [Flarum 2.0] Remove the code below. + // This code prevents extensions which do not implement all abstract methods of this class from breaking + // the forum frontend. Without it, function calls would would error rather than returning `undefined.` + + const missingMethods: string[] = []; + + ['className', 'title', 'content', 'onsubmit'].forEach((method) => { + if (!(this as any)[method]) { + (this as any)[method] = function (): void {}; + missingMethods.push(method); + } + }); + + if (missingMethods.length > 0) { + fireDebugWarning( + `Modal \`${this.constructor.name}\` does not implement all abstract methods of the Modal super class. Missing methods: ${missingMethods.join( + ', ' + )}.` + ); + } + } + oncreate(vnode: Mithril.VnodeDOM) { super.oncreate(vnode); From 9a5062eb136da2f9b0d8aea5067121682bbc2bac Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Tue, 21 Sep 2021 09:57:20 +0100 Subject: [PATCH 10/11] Add missing `app` import --- js/src/common/helpers/fireDebugWarning.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js/src/common/helpers/fireDebugWarning.ts b/js/src/common/helpers/fireDebugWarning.ts index 8c58e79863..159dfca1ad 100644 --- a/js/src/common/helpers/fireDebugWarning.ts +++ b/js/src/common/helpers/fireDebugWarning.ts @@ -1,3 +1,5 @@ +import app from '../app'; + /** * Calls `console.warn` with the provided arguments, but only if the forum is in debug mode. * From c64ecb535dae13d01c4a09e924989ac97ab4cbd9 Mon Sep 17 00:00:00 2001 From: David Wheatley Date: Wed, 20 Oct 2021 21:21:56 +0100 Subject: [PATCH 11/11] Address review comment Co-authored-by: David Sevilla Martin <6401250+datitisev@users.noreply.github.com> --- js/src/common/components/Modal.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/common/components/Modal.tsx b/js/src/common/components/Modal.tsx index b5cc33f83d..8391b24cf4 100644 --- a/js/src/common/components/Modal.tsx +++ b/js/src/common/components/Modal.tsx @@ -59,7 +59,7 @@ export default abstract class Modal extends Component) { super.oncreate(vnode); - this.attrs.animateShow(this.onready); + this.attrs.animateShow(() => this.onready()); } onbeforeremove(vnode: Mithril.VnodeDOM): Promise | void {