From c2d94be06e4fffec1b333881046c293d5e39beb5 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 21 Nov 2023 16:40:10 +0000 Subject: [PATCH] DEV: Make loading spinner implementation consistent with slider (#24480) In the past, our loading spinner implementation used Ember's loading substate. That meant that, when the site setting was toggled, there would be fundamental changes in the routing behavior. This commit simplifies things so that the (non-default) loading spinner implementation is purely a styling thing, and behaves exactly the same as the spinner which appears under the 'slider' configuration when loading takes too long. This does involve a slight UX change. Now, the entire page will be replaced by a loading spinner instead of just the relevant `{{outlet}}`. We strongly recommend sites use the new default 'slider' behavior. --- .../loading-slider-fallback-spinner.hbs | 6 ++++-- .../components/loading-slider-fallback-spinner.js | 7 +++++++ .../app/components/page-loading-slider.hbs | 2 +- .../discourse/app/components/topic-list-item.js | 8 -------- .../discourse/app/routes/application.js | 14 +++++--------- .../discourse/app/services/loading-slider.js | 4 ++-- .../tests/acceptance/loading-indicator-test.js | 11 +++++++---- app/assets/stylesheets/common/loading-slider.scss | 15 ++++----------- 8 files changed, 30 insertions(+), 37 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.hbs b/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.hbs index 629092f1ce0..2f7f9d1ba5f 100644 --- a/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.hbs +++ b/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.hbs @@ -1,3 +1,5 @@ -{{#if this.loadingSlider.stillLoading}} - +{{#if this.shouldDisplay}} + + {{body-class "has-route-loading-spinner"}} + {{hide-application-footer}} {{/if}} \ No newline at end of file diff --git a/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.js b/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.js index 5cb2fddc892..45a4a995cff 100644 --- a/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.js +++ b/app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.js @@ -3,4 +3,11 @@ import { inject as service } from "@ember/service"; export default class LoadingSliderFallbackSpinner extends Component { @service loadingSlider; + + get shouldDisplay() { + const { mode, loading, stillLoading } = this.loadingSlider; + return ( + (mode === "spinner" && loading) || (mode === "slider" && stillLoading) + ); + } } diff --git a/app/assets/javascripts/discourse/app/components/page-loading-slider.hbs b/app/assets/javascripts/discourse/app/components/page-loading-slider.hbs index f895371072f..6393272cd34 100644 --- a/app/assets/javascripts/discourse/app/components/page-loading-slider.hbs +++ b/app/assets/javascripts/discourse/app/components/page-loading-slider.hbs @@ -1,4 +1,4 @@ -{{#if this.loadingSlider.enabled}} +{{#if (eq this.loadingSlider.mode "slider")}}
{ - this.loadingSlider.transitionEnded(); - }); - return false; - } else { - return true; // Use native ember loading implementation - } + this.loadingSlider.transitionStarted(); + transition.finally(() => { + this.loadingSlider.transitionEnded(); + }); + return false; }, actions: { diff --git a/app/assets/javascripts/discourse/app/services/loading-slider.js b/app/assets/javascripts/discourse/app/services/loading-slider.js index 850a2ee644e..2150bf3f6b7 100644 --- a/app/assets/javascripts/discourse/app/services/loading-slider.js +++ b/app/assets/javascripts/discourse/app/services/loading-slider.js @@ -74,8 +74,8 @@ export default class LoadingSlider extends Service.extend(Evented) { timer = new Timer(); - get enabled() { - return this.siteSettings.page_loading_indicator === "slider"; + get mode() { + return this.siteSettings.page_loading_indicator; } get averageLoadingDuration() { diff --git a/app/assets/javascripts/discourse/tests/acceptance/loading-indicator-test.js b/app/assets/javascripts/discourse/tests/acceptance/loading-indicator-test.js index af62e697e3d..9efa91019a8 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/loading-indicator-test.js +++ b/app/assets/javascripts/discourse/tests/acceptance/loading-indicator-test.js @@ -11,6 +11,9 @@ import AboutFixtures from "discourse/tests/fixtures/about"; import pretender from "discourse/tests/helpers/create-pretender"; import { acceptance, query } from "discourse/tests/helpers/qunit-helpers"; +const SPINNER_SELECTOR = + "#main-outlet-wrapper .route-loading-spinner div.spinner"; + // Like settled(), but ignores timers, transitions and network requests function isMostlySettled() { let { hasRunLoop, hasPendingWaiters, isRenderPending } = getSettledState(); @@ -54,15 +57,15 @@ acceptance("Page Loading Indicator", function (needs) { const aboutRequest = await pendingRequest; await mostlySettled(); - assert.strictEqual(currentRouteName(), "about_loading"); - assert.dom("#main-outlet > div.spinner").exists(); + assert.strictEqual(currentRouteName(), "discovery.latest"); + assert.dom(SPINNER_SELECTOR).exists(); assert.dom(".loading-indicator-container").doesNotExist(); pretender.resolve(aboutRequest); await settled(); assert.strictEqual(currentRouteName(), "about"); - assert.dom("#main-outlet > div.spinner").doesNotExist(); + assert.dom(SPINNER_SELECTOR).doesNotExist(); assert.dom("#main-outlet section.about").exists(); }); @@ -80,7 +83,7 @@ acceptance("Page Loading Indicator", function (needs) { await mostlySettled(); assert.strictEqual(currentRouteName(), "discovery.latest"); - assert.dom("#main-outlet > div.spinner").doesNotExist(); + assert.dom(SPINNER_SELECTOR).doesNotExist(); await waitUntil(() => query(".loading-indicator-container").classList.contains("loading") diff --git a/app/assets/stylesheets/common/loading-slider.scss b/app/assets/stylesheets/common/loading-slider.scss index dc4be5e46aa..9f37ee66da9 100644 --- a/app/assets/stylesheets/common/loading-slider.scss +++ b/app/assets/stylesheets/common/loading-slider.scss @@ -58,17 +58,10 @@ } } -.loading-slider-fallback-spinner { +.route-loading-spinner { padding-top: 1.8em; +} + +body.has-route-loading-spinner #main-outlet { display: none; } - -body.still-loading { - .loading-slider-fallback-spinner { - display: block; - } - - #main-outlet { - display: none; - } -}