DEV: A general code clean-up (#49)

Summary:

* removed `computed` usage from a glimmer component
* converted the component to gjs
* converted the connector to gjs and made it into a regular glimmer component (w/ service injections and a getter instead of `setupComponent`)
* used `session` service injection instead of Session singleton wherever possible
* moved prop initialization out of a constructor
* removed a use of string-based action
* simplified showInSidebar logic
* converted the initializer to the new class-based paradigm (w/ service injections)
* added an escape hatch (isDestroying/isDestroyed check) to a `later` call
* add a matching `removeEventListener` to `window.matchMedia(…).addEventListener`
* used qunit-dom in tests

javascripts/discourse/components/color-scheme-toggler.js → javascripts/discourse/components/color-scheme-toggler.gjs

@@ -1,23 +1,21 @@
 import Component from "@glimmer/component";
 import { tracked } from "@glimmer/tracking";
-import { action, computed } from "@ember/object";
+import { action } from "@ember/object";
 import { service } from "@ember/service";
-import Session from "discourse/models/session";
+import DButton from "discourse/components/d-button";
+import i18n from "discourse-common/helpers/i18n";
 import {
   COLOR_SCHEME_OVERRIDE_KEY,
   colorSchemeOverride,
 } from "../lib/color-scheme-override";
 
 export default class ColorSchemeToggler extends Component {
   @service keyValueStore;
-  @tracked storedOverride;
+  @service session;
 
-  constructor() {
-    super(...arguments);
-    this.storedOverride = this.keyValueStore.getItem(COLOR_SCHEME_OVERRIDE_KEY);
-  }
+  @tracked
+  storedOverride = this.keyValueStore.getItem(COLOR_SCHEME_OVERRIDE_KEY);
 
-  @computed("storedOverride")
   get toggleButtonIcon() {
     switch (this.OSMode) {
       case "dark":
@@ -56,8 +54,17 @@ export default class ColorSchemeToggler extends Component {
       this.keyValueStore.getItem(COLOR_SCHEME_OVERRIDE_KEY) || null;
 
     // currently only used to flip category logos back/forth
-    Session.currentProp("colorSchemeOverride", this.storedOverride);
+    this.session.set("colorSchemeOverride", this.storedOverride);
 
     colorSchemeOverride(this.storedOverride);
   }
+
+  <template>
+    <DButton
+      @action={{this.toggleScheme}}
+      @icon={{this.toggleButtonIcon}}
+      @translatedTitle={{i18n (themePrefix "toggle_button_title")}}
+      class="color-scheme-toggler btn-flat"
+    />
+  </template>
 }

javascripts/discourse/connectors/sidebar-footer-actions/toggler-button.gjs

@@ -0,0 +1,22 @@
+import Component from "@glimmer/component";
+import { service } from "@ember/service";
+import ColorSchemeToggler from "../../components/color-scheme-toggler";
+
+export default class TogglerButton extends Component {
+  @service session;
+  @service siteSettings;
+
+  get showInSidebar() {
+    return (
+      (this.session.darkModeAvailable ||
+        this.siteSettings.default_dark_mode_color_scheme_id >= 0) &&
+      !settings.add_color_scheme_toggle_to_header
+    );
+  }
+
+  <template>
+    {{#if this.showInSidebar}}
+      <ColorSchemeToggler />
+    {{/if}}
+  </template>
+}

javascripts/discourse/initializers/color-scheme-toggler.gjs

@@ -1,58 +1,69 @@
+import { setOwner } from "@ember/owner";
 import { later, schedule } from "@ember/runloop";
+import { service } from "@ember/service";
 import { loadColorSchemeStylesheet } from "discourse/lib/color-scheme-picker";
 import { withPluginApi } from "discourse/lib/plugin-api";
 import { currentThemeId } from "discourse/lib/theme-selector";
-import Session from "discourse/models/session";
+import { bind } from "discourse-common/utils/decorators";
 import ColorSchemeToggler from "../components/color-scheme-toggler";
 import {
   COLOR_SCHEME_OVERRIDE_KEY,
   colorSchemeOverride,
 } from "../lib/color-scheme-override";
 
-export default {
-  name: "color-scheme-toggler",
+class TogglerInit {
+  @service keyValueStore;
+  @service session;
+  @service siteSettings;
 
-  initialize(container) {
-    const keyValueStore = container.lookup("service:key-value-store");
-    const storedOverride = keyValueStore.getItem(COLOR_SCHEME_OVERRIDE_KEY);
+  constructor(owner) {
+    setOwner(this, owner);
 
-    if (!Session.currentProp("darkModeAvailable")) {
-      const siteSettings = container.lookup("service:site-settings");
+    const storedOverride = this.keyValueStore.getItem(
+      COLOR_SCHEME_OVERRIDE_KEY
+    );
 
-      if (siteSettings.default_dark_mode_color_scheme_id > 0) {
-        loadColorSchemeStylesheet(
-          siteSettings.default_dark_mode_color_scheme_id,
-          currentThemeId(),
-          true
-        ).then(() => {
-          if (storedOverride) {
-            colorSchemeOverride(storedOverride);
-          } else {
-            // ensures that this extra stylesheet isn't auto-used when OS is in dark mode
-            document.querySelector("link#cs-preview-dark").media =
-              "(prefers-color-scheme: none)";
-          }
-        });
-      } else {
+    if (!this.session.darkModeAvailable) {
+      if (this.siteSettings.default_dark_mode_color_scheme_id <= 0) {
         // eslint-disable-next-line no-console
         console.warn(
           "No dark color scheme available, the discourse-color-scheme-toggle component has no effect."
         );
         return;
       }
+
+      loadColorSchemeStylesheet(
+        this.siteSettings.default_dark_mode_color_scheme_id,
+        currentThemeId(),
+        true
+      ).then(() => {
+        if (storedOverride) {
+          colorSchemeOverride(storedOverride);
+        } else {
+          // ensures that this extra stylesheet isn't auto-used when OS is in dark mode
+          document.querySelector("link#cs-preview-dark").media =
+            "(prefers-color-scheme: none)";
+        }
+      });
     }
 
     if (storedOverride) {
-      Session.currentProp("colorSchemeOverride", storedOverride);
+      this.session.set("colorSchemeOverride", storedOverride);
     }
 
-    if (Session.currentProp("darkModeAvailable") && storedOverride) {
+    if (this.session.darkModeAvailable && storedOverride) {
       schedule("afterRender", () => {
         const logoDarkSrc = document.querySelector(".title picture source");
         // in some cases the logo widget is not yet rendered
         // so we schedule the calculation after a short delay
         if (!logoDarkSrc) {
-          later(() => colorSchemeOverride(storedOverride), 500);
+          later(() => {
+            if (owner.isDestroying || owner.isDestroyed) {
+              return;
+            }
+
+            colorSchemeOverride(storedOverride);
+          }, 500);
         } else {
           colorSchemeOverride(storedOverride);
         }
@@ -61,12 +72,7 @@ export default {
 
     window
       .matchMedia("(prefers-color-scheme: dark)")
-      .addEventListener("change", () => {
-        // reset when switching OS dark mode
-        keyValueStore.removeItem(COLOR_SCHEME_OVERRIDE_KEY);
-        Session.currentProp("colorSchemeOverride", null);
-        colorSchemeOverride();
-      });
+      .addEventListener("change", this.onColorChange);
 
     if (settings.add_color_scheme_toggle_to_header) {
       withPluginApi("1.28.0", (api) => {
@@ -83,5 +89,32 @@ export default {
         );
       });
     }
+  }
+
+  @bind
+  onColorChange() {
+    // reset when switching OS dark mode
+    this.keyValueStore.removeItem(COLOR_SCHEME_OVERRIDE_KEY);
+    this.session.set("colorSchemeOverride", null);
+    colorSchemeOverride();
+  }
+
+  teardown() {
+    window
+      .matchMedia("(prefers-color-scheme: dark)")
+      .removeEventListener("change", this.onColorChange);
+  }
+}
+
+export default {
+  name: "color-scheme-toggler",
+
+  initialize(owner) {
+    this.instance = new TogglerInit(owner);
+  },
+
+  teardown() {
+    this.instance.teardown();
+    this.instance = null;
   },
 };

test/acceptance/toggle-test.js

@@ -1,40 +1,38 @@
 import { visit } from "@ember/test-helpers";
 import { test } from "qunit";
 import Session from "discourse/models/session";
-import { acceptance, visible } from "discourse/tests/helpers/qunit-helpers";
+import { acceptance } from "discourse/tests/helpers/qunit-helpers";
 
 acceptance("Color Scheme Toggle - header icon", function (needs) {
-  needs.hooks.beforeEach(() => {
+  needs.hooks.beforeEach(function () {
     settings.add_color_scheme_toggle_to_header = true;
-    Session.currentProp("darkModeAvailable", true);
+    Session.current().set("darkModeAvailable", true);
   });
 
-  needs.hooks.afterEach(() => {
-    Session.currentProp("darkModeAvailable", null);
+  needs.hooks.afterEach(function () {
+    Session.current().set("darkModeAvailable", null);
   });
 
   test("shows in header", async function (assert) {
     await visit("/");
 
-    assert.ok(
-      visible(".header-color-scheme-toggle"),
-      "button present in header"
-    );
+    assert
+      .dom(".header-color-scheme-toggle")
+      .exists("button present in header");
   });
 });
 
 acceptance("Color Scheme Toggle - no op", function (needs) {
-  needs.hooks.beforeEach(() => {
+  needs.hooks.beforeEach(function () {
     settings.add_color_scheme_toggle_to_header = true;
   });
 
   test("does not show when no dark color scheme available", async function (assert) {
     await visit("/");
 
-    assert.ok(
-      !visible(".header-color-scheme-toggle"),
-      "button is not present in header"
-    );
+    assert
+      .dom(".header-color-scheme-toggle")
+      .doesNotExist("button is not present in header");
   });
 });
 
@@ -44,25 +42,25 @@ acceptance("Color Scheme Toggle - sidebar icon", function (needs) {
     enable_experimental_sidebar_hamburger: true,
   });
 
-  needs.hooks.beforeEach(() => {
+  needs.hooks.beforeEach(function () {
     settings.add_color_scheme_toggle_to_header = false;
-    Session.currentProp("darkModeAvailable", true);
+    Session.current().set("darkModeAvailable", true);
   });
 
-  needs.hooks.afterEach(() => {
-    Session.currentProp("darkModeAvailable", null);
+  needs.hooks.afterEach(function () {
+    Session.current().set("darkModeAvailable", null);
   });
 
   test("shows in sidebar", async function (assert) {
     await visit("/");
 
-    assert.ok(
-      !visible(".header-color-scheme-toggle"),
-      "button not present in header"
-    );
+    assert
+      .dom(".header-color-scheme-toggle")
+      .doesNotExist("button not present in header");
 
-    const toggleButton = ".sidebar-footer-wrapper .color-scheme-toggler";
-    assert.ok(visible(toggleButton), "button in footer");
+    assert
+      .dom(".sidebar-footer-wrapper .color-scheme-toggler")
+      .exists("button in footer");
   });
 });
 
@@ -83,19 +81,19 @@ acceptance("Color Scheme Toggle - sidebar icon", function (needs) {
     default_dark_mode_color_scheme_id: 2,
   });
 
-  needs.hooks.beforeEach(() => {
+  needs.hooks.beforeEach(function () {
     settings.add_color_scheme_toggle_to_header = false;
   });
 
   test("shows in sidebar if site has auto dark mode", async function (assert) {
     await visit("/");
 
-    assert.ok(
-      !visible(".header-color-scheme-toggle"),
-      "button not present in header"
-    );
+    assert
+      .dom(".header-color-scheme-toggle")
+      .doesNotExist("button not present in header");
 
-    const toggleButton = ".sidebar-footer-wrapper .color-scheme-toggler";
-    assert.ok(visible(toggleButton), "button in footer");
+    assert
+      .dom(".sidebar-footer-wrapper .color-scheme-toggler")
+      .exists("button in footer");
   });
 });