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.

app/assets/javascripts/discourse/app/components/loading-slider-fallback-spinner.hbs

@@ -1,3 +1,5 @@
-{{#if this.loadingSlider.stillLoading}}
-  <div class="loading-slider-fallback-spinner">{{loading-spinner}}</div>
+{{#if this.shouldDisplay}}
+  <div class="route-loading-spinner">{{loading-spinner}}</div>
+  {{body-class "has-route-loading-spinner"}}
+  {{hide-application-footer}}
 {{/if}}

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)
+    );
+  }
 }

app/assets/javascripts/discourse/app/components/page-loading-slider.hbs

@@ -1,4 +1,4 @@
-{{#if this.loadingSlider.enabled}}
+{{#if (eq this.loadingSlider.mode "slider")}}
   <div
     class={{concat-class
       "loading-indicator-container"

app/assets/javascripts/discourse/app/components/topic-list-item.js

@@ -40,14 +40,6 @@ export function navigateToTopic(topic, href) {
   const owner = getOwner(this);
   const router = owner.lookup("service:router");
   const session = owner.lookup("service:session");
-  const siteSettings = owner.lookup("service:site-settings");
-  const appEvents = owner.lookup("service:appEvents");
-
-  if (siteSettings.page_loading_indicator !== "slider") {
-    // With the slider, it feels nicer for the header to update once the rest of the topic content loads,
-    // so skip setting it early.
-    appEvents.trigger("header:update-topic", topic);
-  }
 
   session.set("lastTopicIdViewed", {
     topicId: topic.id,

app/assets/javascripts/discourse/app/routes/application.js

@@ -56,15 +56,11 @@ const ApplicationRoute = DiscourseRoute.extend({
 
   @action
   loading(transition) {
-    if (this.loadingSlider.enabled) {
-      this.loadingSlider.transitionStarted();
-      transition.promise.finally(() => {
-        this.loadingSlider.transitionEnded();
-      });
-      return false;
-    } else {
-      return true; // Use native ember loading implementation
-    }
+    this.loadingSlider.transitionStarted();
+    transition.finally(() => {
+      this.loadingSlider.transitionEnded();
+    });
+    return false;
   },
 
   actions: {

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() {

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")

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;
-  }
-}