From 0c95d28e948dafadcc4abedaae2b81b942d56eda Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sun, 26 Dec 2021 20:04:48 -0500 Subject: [PATCH 1/4] Fix Search error when user can't search If there are no search sources, HTML for the Search component won't be rendered, so trying to attach listeners to it will likely error. In this PR, we don't attach such listeners/logic if there are no sources. We also stop asserting that sources is defined to help avoid other similar issues in the future. --- js/src/forum/components/Search.tsx | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/js/src/forum/components/Search.tsx b/js/src/forum/components/Search.tsx index 645fa9241..054c0b307 100644 --- a/js/src/forum/components/Search.tsx +++ b/js/src/forum/components/Search.tsx @@ -88,7 +88,7 @@ export default class Search extends Compone /** * An array of SearchSources. */ - protected sources!: SearchSource[]; + protected sources?: SearchSource[]; /** * The number of sources that are still loading results. @@ -192,7 +192,7 @@ export default class Search extends Compone this.setIndex(this.getCurrentNumericIndex()); // If there are no sources, the search view is not shown. - if (!this.sources.length) return; + if (!this.sources?.length) return; this.updateMaxHeight(); } @@ -200,6 +200,10 @@ export default class Search extends Compone oncreate(vnode: Mithril.VnodeDOM) { super.oncreate(vnode); + // If there are no sources, we shouldn't initialize logic for + // search elements, as they will not be shown. + if (!this.sources?.length) return; + const search = this; const state = this.searchState; @@ -237,7 +241,7 @@ export default class Search extends Compone if (state.isCached(query)) return; if (query.length >= (search.constructor as typeof Search).MIN_SEARCH_LEN) { - search.sources.map((source) => { + search.sources?.map((source) => { if (!source.search) return; search.loadingSources++; From d89031f0573e504bd7d085d8429383435d60f062 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sun, 26 Dec 2021 22:45:58 -0500 Subject: [PATCH 2/4] Fix drawer focus trap making login form unclickable on mobile Adding `clickOutsideDeactivates` seems to fix the issue, contrary to what the focus-trap documentation implies about it being unnecessary. --- js/src/common/utils/Drawer.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/js/src/common/utils/Drawer.js b/js/src/common/utils/Drawer.js index c0003dd1e..23f5e5e22 100644 --- a/js/src/common/utils/Drawer.js +++ b/js/src/common/utils/Drawer.js @@ -27,7 +27,10 @@ export default class Drawer { }); this.appElement = document.getElementById('app'); - this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true }); + // Despite the `focus-trap` documentation, both `clickOutsideDeactivates` + // and `allowOutsideClick` are necessary so that inputs in modals triggered + // from the drawer's nav components can be interacted with. + this.focusTrap = createFocusTrap('#drawer', { allowOutsideClick: true, clickOutsideDeactivates: true }); this.drawerAvailableMediaQuery = window.matchMedia( `(max-width: ${getComputedStyle(document.documentElement).getPropertyValue('--screen-phone-max')})` ); From a55b61e058ae142d2c68dd6ef328770cd232c93c Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Mon, 27 Dec 2021 18:15:12 -0500 Subject: [PATCH 3/4] Use translations for page titles in frontend This gives more flexibility for customization, and allows overriding title structure via translations / linguist. --- js/src/common/Application.tsx | 19 ++++++++++++++++--- locale/core.yml | 10 +++++----- src/Frontend/Driver/BasicTitleDriver.php | 4 ++-- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/js/src/common/Application.tsx b/js/src/common/Application.tsx index 3dc7b8259..09a13034d 100644 --- a/js/src/common/Application.tsx +++ b/js/src/common/Application.tsx @@ -9,6 +9,7 @@ import Translator from './Translator'; import Store, { ApiPayload, ApiResponse, ApiResponsePlural, ApiResponseSingle, payloadIsPlural } from './Store'; import Session from './Session'; import extract from './utils/extract'; +import extractText from './utils/extractText'; import Drawer from './utils/Drawer'; import mapRoutes from './utils/mapRoutes'; import RequestError, { InternalFlarumRequestOptions } from './utils/RequestError'; @@ -365,9 +366,21 @@ export default class Application { updateTitle(): void { const count = this.titleCount ? `(${this.titleCount}) ` : ''; - const pageTitleWithSeparator = this.title && m.route.get() !== this.forum.attribute('basePath') + '/' ? this.title + ' - ' : ''; - const title = this.forum.attribute('title'); - document.title = count + pageTitleWithSeparator + title; + const onHomepage = m.route.get() === this.forum.attribute('basePath') + '/'; + + const params = { + pageTitle: this.title, + forumName: this.forum.attribute('title'), + // Until we add page numbers to the frontend, this is constant at 1 + // so that the page number portion doesn't show up in the URL. + pageNumber: 1, + }; + + const title = + onHomepage || !this.title + ? extractText(app.translator.trans('core.lib.meta_titles.without_page_title', params)) + : extractText(app.translator.trans('core.lib.meta_titles.with_page_title', params)); + document.title = count + title; } protected transformRequestOptions(flarumOptions: FlarumRequestOptions): InternalFlarumRequestOptions { diff --git a/locale/core.yml b/locale/core.yml index 7c4812140..b0e0b055e 100644 --- a/locale/core.yml +++ b/locale/core.yml @@ -542,6 +542,11 @@ core: loading_indicator: accessible_label: => core.ref.loading + # Translations in this namespace are used to format page meta titles. + meta_titles: + with_page_title: "{pageNumber, plural, =1 {{pageTitle} - {forumName}} other {{pageTitle}: Page # - {forumName}}}" + without_page_title: "{pageNumber, plural, =1 {{forumName}} other {Page # - {forumName}}}" + # These translations are used in modals. modal: close: Close @@ -628,11 +633,6 @@ core: submit_button: => core.ref.save_changes title: => core.ref.reset_your_password - # Translations in this namespace are used to format page meta titles. - meta_titles: - with_page_title: "{pageNumber, plural, =1 {{pageTitle} - {forumName}} other {{pageTitle}: Page # - {forumName}}}" - without_page_title: "{pageNumber, plural, =1 {{forumName}} other {Page # - {forumName}}}" - # Translations in this namespace are used in messages output by the API. api: invalid_username_message: "The username may only contain letters, numbers, and dashes." diff --git a/src/Frontend/Driver/BasicTitleDriver.php b/src/Frontend/Driver/BasicTitleDriver.php index b87272149..36929602e 100644 --- a/src/Frontend/Driver/BasicTitleDriver.php +++ b/src/Frontend/Driver/BasicTitleDriver.php @@ -37,7 +37,7 @@ class BasicTitleDriver implements TitleDriverInterface ]; return $onHomePage || ! $document->title - ? $this->translator->trans('core.views.meta_titles.without_page_title', $params) - : $this->translator->trans('core.views.meta_titles.with_page_title', $params); + ? $this->translator->trans('core.lib.meta_titles.without_page_title', $params) + : $this->translator->trans('core.lib.meta_titles.with_page_title', $params); } } From b7f2fe2429cd56b2d417b20469dc861b38988e05 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Mon, 27 Dec 2021 18:28:11 -0500 Subject: [PATCH 4/4] Fix consecutive shows of same modal with different attrs We need to specify a unique key for each modal so that the modals are fully destroyed and recreated. For instance, this fixes the signup modal being empty with OAuth register flows. --- js/src/common/components/ModalManager.tsx | 17 ++++++++++------- js/src/common/states/ModalManagerState.ts | 9 ++++++++- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/js/src/common/components/ModalManager.tsx b/js/src/common/components/ModalManager.tsx index d46899eec..8db7e25bf 100644 --- a/js/src/common/components/ModalManager.tsx +++ b/js/src/common/components/ModalManager.tsx @@ -24,16 +24,19 @@ export default class ModalManager extends Component { view(vnode: Mithril.VnodeDOM): Mithril.Children { const modal = this.attrs.state.modal; + const Tag = modal?.componentClass; return (
- {!!modal && - modal.componentClass.component({ - ...modal.attrs, - animateShow: this.animateShow.bind(this), - animateHide: this.animateHide.bind(this), - state: this.attrs.state, - })} + {!!Tag && ( + + )}
); } diff --git a/js/src/common/states/ModalManagerState.ts b/js/src/common/states/ModalManagerState.ts index 425de5150..30884c5e3 100644 --- a/js/src/common/states/ModalManagerState.ts +++ b/js/src/common/states/ModalManagerState.ts @@ -22,8 +22,15 @@ export default class ModalManagerState { modal: null | { componentClass: UnsafeModalClass; attrs?: Record; + key: number; } = null; + /** + * Used to force re-initialization of modals if a modal + * is replaced by another of the same type. + */ + private key = 0; + private closeTimeout?: NodeJS.Timeout; /** @@ -48,7 +55,7 @@ export default class ModalManagerState { if (this.closeTimeout) clearTimeout(this.closeTimeout); - this.modal = { componentClass, attrs }; + this.modal = { componentClass, attrs, key: this.key++ }; m.redraw.sync(); }