From 505ae7493ea3fa3caabf00ba587a87eb4af95158 Mon Sep 17 00:00:00 2001 From: Kevin Ansfield Date: Fri, 22 Oct 2021 10:46:12 +0100 Subject: [PATCH] Optimized loading of custom theme setting in design screens no issue We want to automatically show brand settings expanded in the design menu when the active theme has no custom theme settings, in order to do that without causing visual noise/jank we need to ensure that we have all the data we need up-front before the design menu is rendered. - optimized `customThemeSettings` loading behaviour - `.load()` will now only perform a fetch if settings have not previously been loaded so it can be called without causing unnecessary waits - `.reload()` will force a clear+refetch of the settings - called by `themeManagement.activate()` after successfully changing a theme - moved fetching of theme settings from the design menu constructor to the `settings.design` route's `model()` hook - means the app will wait for loading to finish before showing any of the design settings screen so we can guarantee the data we need is available - moved update of preview html from the design menu constructor to the design settings route as it's a more appropriate place to find screen setup/loading behaviour --- ghost/admin/app/components/gh-nav-menu/design.js | 8 -------- ghost/admin/app/routes/settings/design.js | 13 ++++++++++++- ghost/admin/app/routes/settings/design/index.js | 2 +- ghost/admin/app/services/custom-theme-settings.js | 14 ++++++++++++++ ghost/admin/app/services/theme-management.js | 2 +- 5 files changed, 28 insertions(+), 11 deletions(-) diff --git a/ghost/admin/app/components/gh-nav-menu/design.js b/ghost/admin/app/components/gh-nav-menu/design.js index 6aef53eb68..baee18904c 100644 --- a/ghost/admin/app/components/gh-nav-menu/design.js +++ b/ghost/admin/app/components/gh-nav-menu/design.js @@ -1,7 +1,6 @@ import Component from '@glimmer/component'; import {action} from '@ember/object'; import {inject as service} from '@ember/service'; -import {task} from 'ember-concurrency-decorators'; import {tracked} from '@glimmer/tracking'; export default class DesignMenuComponent extends Component { @@ -18,8 +17,6 @@ export default class DesignMenuComponent extends Component { constructor() { super(...arguments); - this.fetchThemeSettingsTask.perform(); - this.themeManagement.updatePreviewHtmlTask.perform(); // fetch all themes in the background so we can show the active theme this.store.findAll('theme'); @@ -45,11 +42,6 @@ export default class DesignMenuComponent extends Component { } } - @task - *fetchThemeSettingsTask() { - yield this.customThemeSettings.load(); - } - @action transitionBackToIndex() { if (this.router.currentRouteName !== 'settings.design.index') { diff --git a/ghost/admin/app/routes/settings/design.js b/ghost/admin/app/routes/settings/design.js index b9c977de53..96a6a191de 100644 --- a/ghost/admin/app/routes/settings/design.js +++ b/ghost/admin/app/routes/settings/design.js @@ -2,9 +2,11 @@ import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; import {inject as service} from '@ember/service'; export default class SettingsDesignRoute extends AuthenticatedRoute { + @service customThemeSettings; @service feature; @service modals; @service settings; + @service themeManagement; @service ui; beforeModel() { @@ -20,7 +22,16 @@ export default class SettingsDesignRoute extends AuthenticatedRoute { } model() { - return this.settings.reload(); + // background refresh of preview + // not doing it on the 'index' route so that we don't reload going to/from the index, + // any actions performed on child routes that need a refresh should trigger it explicitly + this.themeManagement.updatePreviewHtmlTask.perform(); + + // wait for settings to be loaded - we need the data to be present before display + return Promise.all([ + this.settings.reload(), + this.customThemeSettings.load() + ]); } activate() { diff --git a/ghost/admin/app/routes/settings/design/index.js b/ghost/admin/app/routes/settings/design/index.js index e8d69b4a60..7fc588a481 100644 --- a/ghost/admin/app/routes/settings/design/index.js +++ b/ghost/admin/app/routes/settings/design/index.js @@ -2,7 +2,7 @@ import AuthenticatedRoute from 'ghost-admin/routes/authenticated'; import {action} from '@ember/object'; import {inject as service} from '@ember/service'; -export default class SettingsDesignRoute extends AuthenticatedRoute { +export default class SettingsDesignIndexRoute extends AuthenticatedRoute { @service customThemeSettings; @service modals; @service settings; diff --git a/ghost/admin/app/services/custom-theme-settings.js b/ghost/admin/app/services/custom-theme-settings.js index c6ce60ed8b..537d5831fb 100644 --- a/ghost/admin/app/services/custom-theme-settings.js +++ b/ghost/admin/app/services/custom-theme-settings.js @@ -11,6 +11,8 @@ export default class CustomThemeSettingsServices extends Service { @tracked settings = []; @tracked settingGroups = []; + _hasLoaded = false; + KNOWN_GROUPS = [{ key: 'homepage', name: 'Homepage', @@ -42,8 +44,18 @@ export default class CustomThemeSettingsServices extends Service { return this.loadTask.perform(); } + reload() { + this._hasLoaded = false; + + return this.loadTask.perform(); + } + @task *loadTask() { + if (this.hasLoaded) { + return this.settings; + } + // unload stored settings and re-load from API so they always match active theme // run is required here, see https://github.com/emberjs/data/issues/5447#issuecomment-845672812 run(() => this.store.unloadAll('custom-theme-setting')); @@ -52,6 +64,8 @@ export default class CustomThemeSettingsServices extends Service { this.settings = settings; this.settingGroups = this._buildSettingGroups(settings); + this._hasLoaded = true; + return this.settings; } diff --git a/ghost/admin/app/services/theme-management.js b/ghost/admin/app/services/theme-management.js index 4aef96ac1e..8339704a61 100644 --- a/ghost/admin/app/services/theme-management.js +++ b/ghost/admin/app/services/theme-management.js @@ -90,7 +90,7 @@ export default class ThemeManagementService extends Service { const activatedTheme = yield theme.activate(); this.updatePreviewHtmlTask.perform(); - this.customThemeSettings.load(); + this.customThemeSettings.reload(); if (!options.skipErrors) { const {warnings, errors} = activatedTheme;