UX: Improve color scheme choices in user prefs (#11656)

app/assets/javascripts/discourse/app/controllers/preferences/interface.js

@@ -11,11 +11,11 @@ import {
   updateColorSchemeCookie,
 } from "discourse/lib/color-scheme-picker";
 import { listThemes, setLocalTheme } from "discourse/lib/theme-selector";
+import { not, reads } from "@ember/object/computed";
 import I18n from "I18n";
 import { computed } from "@ember/object";
 import discourseComputed from "discourse-common/utils/decorators";
 import { popupAjaxError } from "discourse/lib/ajax-error";
-import { reads } from "@ember/object/computed";
 import { reload } from "discourse/helpers/page-reloader";
 
 const USER_HOMES = {
@@ -33,7 +33,6 @@ const TITLE_COUNT_MODES = ["notifications", "contextual"];
 export default Controller.extend({
   currentThemeId: -1,
   previewingColorScheme: false,
-  selectedColorSchemeId: null,
   selectedDarkColorSchemeId: null,
   preferencesController: inject("preferences"),
   makeColorSchemeDefault: true,
@@ -41,10 +40,7 @@ export default Controller.extend({
   init() {
     this._super(...arguments);
 
-    this.setProperties({
+    this.set("selectedDarkColorSchemeId", this.session.userDarkSchemeId);
-      selectedColorSchemeId: this.session.userColorSchemeId,
-      selectedDarkColorSchemeId: this.session.userDarkSchemeId,
-    });
   },
 
   @discourseComputed("makeThemeDefault")
@@ -151,6 +147,23 @@ export default Controller.extend({
     "user.color_schemes.default_description"
   ),
 
+  @discourseComputed(
+    "userSelectableThemes",
+    "userSelectableColorSchemes",
+    "themeId"
+  )
+  currentSchemeCanBeSelected(userThemes, userColorSchemes, themeId) {
+    if (!userThemes) {
+      return false;
+    }
+
+    const currentThemeColorSchemeId = userThemes.findBy("id", themeId)
+      .color_scheme_id;
+    return userColorSchemes.findBy("id", currentThemeColorSchemeId);
+  },
+
+  showColorSchemeNoneItem: not("currentSchemeCanBeSelected"),
+
   @discourseComputed("model.user_option.theme_ids", "themeId")
   showThemeSetDefault(userOptionThemes, selectedTheme) {
     return !userOptionThemes || userOptionThemes[0] !== selectedTheme;
@@ -216,6 +229,17 @@ export default Controller.extend({
     },
   }),
 
+  selectedColorSchemeId: computed({
+    set(key, value) {
+      return value;
+    },
+    get() {
+      return this.currentSchemeCanBeSelected
+        ? this.session.userColorSchemeId
+        : null;
+    },
+  }),
+
   actions: {
     save() {
       this.set("saved", false);

app/assets/javascripts/discourse/app/templates/preferences/interface.hbs

@@ -29,14 +29,14 @@
       {{/if}}
       <div class="controls">
         {{combo-box
-                content=userSelectableColorSchemes
+          content=userSelectableColorSchemes
-                value=selectedColorSchemeId
+          value=selectedColorSchemeId
-                onChange=(action "loadColorScheme")
+          onChange=(action "loadColorScheme")
-                options=(hash
+          options=(hash
-                  translatedNone=selectedColorSchemeNoneLabel
+            translatedNone=selectedColorSchemeNoneLabel
-                )
+            autoInsertNoneItem=showColorSchemeNoneItem
-
+          )
-              }}
+        }}
       </div>
     </div>
     {{#if showDarkColorSchemeSelector}}

app/assets/javascripts/discourse/tests/acceptance/user-preferences-interface-test.js

@@ -5,6 +5,7 @@ import {
 } from "discourse/tests/helpers/qunit-helpers";
 import { click, visit } from "@ember/test-helpers";
 import cookie, { removeCookie } from "discourse/lib/cookie";
+import I18n from "I18n";
 import Session from "discourse/models/session";
 import Site from "discourse/models/site";
 import selectKit from "discourse/tests/helpers/select-kit-helper";
@@ -60,7 +61,7 @@ acceptance("User Preferences - Interface", function (needs) {
 
   test("does not show option to disable dark mode by default", async function (assert) {
     await visit("/u/eviltrout/preferences/interface");
-    assert.equal($(".control-group.dark-mode").length, 0);
+    assert.ok(!exists(".control-group.dark-mode"), "option not visible");
   });
 
   test("shows light/dark color scheme pickers", async function (assert) {
@@ -71,8 +72,65 @@ acceptance("User Preferences - Interface", function (needs) {
     ]);
 
     await visit("/u/eviltrout/preferences/interface");
-    assert.ok($(".light-color-scheme").length, "has regular dropdown");
+    assert.ok(exists(".light-color-scheme"), "has regular dropdown");
-    assert.ok($(".dark-color-scheme").length, "has dark color scheme dropdown");
+    assert.ok(exists(".dark-color-scheme"), "has dark color scheme dropdown");
+  });
+
+  test("shows light color scheme default option when theme's color scheme is not user selectable", async function (assert) {
+    let site = Site.current();
+    site.set("user_themes", [
+      { id: 1, name: "Cool Theme", color_scheme_id: null },
+    ]);
+
+    site.set("user_color_schemes", [{ id: 2, name: "Cool Breeze" }]);
+
+    await visit("/u/eviltrout/preferences/interface");
+    assert.ok(exists(".light-color-scheme"), "has regular dropdown");
+
+    assert.equal(
+      selectKit(".light-color-scheme .select-kit").header().value(),
+      null
+    );
+    assert.equal(
+      selectKit(".light-color-scheme .select-kit").header().label(),
+      I18n.t("user.color_schemes.default_description")
+    );
+  });
+
+  test("shows no default option for light scheme when theme's color scheme is user selectable", async function (assert) {
+    let meta = document.createElement("meta");
+    meta.name = "discourse_theme_ids";
+    meta.content = "2";
+    document.getElementsByTagName("head")[0].appendChild(meta);
+
+    let site = Site.current();
+    site.set("user_themes", [
+      { theme_id: 1, name: "Cool Theme", color_scheme_id: 2, default: true },
+      {
+        theme_id: 2,
+        name: "Some Other Theme",
+        color_scheme_id: 3,
+        default: false,
+      },
+    ]);
+
+    site.set("user_color_schemes", [
+      { id: 2, name: "Cool Breeze" },
+      { id: 3, name: "Dark Night" },
+    ]);
+
+    await visit("/u/eviltrout/preferences/interface");
+
+    assert.ok(exists(".light-color-scheme"), "has regular dropdown");
+    assert.equal(selectKit(".theme .select-kit").header().value(), 2);
+
+    await selectKit(".light-color-scheme .select-kit").expand();
+    assert.equal(
+      queryAll(".light-color-scheme .select-kit .select-kit-row").length,
+      2
+    );
+
+    document.querySelector("meta[name='discourse_theme_ids']").remove();
   });
 });
 
@@ -93,7 +151,7 @@ acceptance(
       await visit("/u/eviltrout/preferences/interface");
 
       assert.ok(
-        $(".control-group.dark-mode").length,
+        exists(".control-group.dark-mode"),
         "it has the option to disable dark mode"
       );
     });
@@ -103,7 +161,7 @@ acceptance(
       site.set("user_color_schemes", []);
 
       await visit("/u/eviltrout/preferences/interface");
-      assert.equal($(".control-group.color-scheme").length, 0);
+      assert.ok(!exists(".control-group.color-scheme"));
     });
 
     test("light color scheme picker", async function (assert) {
@@ -111,10 +169,9 @@ acceptance(
       site.set("user_color_schemes", [{ id: 2, name: "Cool Breeze" }]);
 
       await visit("/u/eviltrout/preferences/interface");
-      assert.ok($(".light-color-scheme").length, "has regular picker dropdown");
+      assert.ok(exists(".light-color-scheme"), "has regular picker dropdown");
-      assert.equal(
+      assert.ok(
-        $(".dark-color-scheme").length,
+        !exists(".dark-color-scheme"),
-        0,
         "does not have a dark color scheme picker"
       );
     });
@@ -138,18 +195,15 @@ acceptance(
       };
 
       await visit("/u/eviltrout/preferences/interface");
-      assert.ok($(".light-color-scheme").length, "has regular dropdown");
+      assert.ok(exists(".light-color-scheme"), "has regular dropdown");
-      assert.ok(
+      assert.ok(exists(".dark-color-scheme"), "has dark color scheme dropdown");
-        $(".dark-color-scheme").length,
-        "has dark color scheme dropdown"
-      );
       assert.equal(
-        $(".dark-color-scheme .selected-name").data("value"),
+        queryAll(".dark-color-scheme .selected-name").data("value"),
         session.userDarkSchemeId,
         "sets site default as selected dark scheme"
       );
-      assert.equal(
+      assert.ok(
-        $(".control-group.dark-mode").length,
+        !exists(".control-group.dark-mode"),
         0,
         "it does not show disable dark mode checkbox"
       );

app/models/theme.rb

@@ -58,7 +58,7 @@ class Theme < ActiveRecord::Base
       theme_fields.select(&:basic_html_field?).each(&:invalidate_baked!)
     end
 
-    Theme.expire_site_cache! if saved_change_to_user_selectable? || saved_change_to_name?
+    Theme.expire_site_cache! if saved_change_to_color_scheme_id? || saved_change_to_user_selectable? || saved_change_to_name?
     notify_with_scheme = saved_change_to_color_scheme_id?
 
     reload

config/locales/client.en.yml

@@ -947,7 +947,7 @@ en:
       color_scheme_default_on_all_devices: "Set default color scheme(s) on all my devices"
       color_scheme: "Color Scheme"
       color_schemes:
-        default_description: "Default"
+        default_description: "Theme default"
         disable_dark_scheme: "Same as regular"
         dark_instructions: "You can preview the dark mode color scheme by toggling your device's dark mode."
         undo: "Reset"

spec/models/site_spec.rb

@@ -21,6 +21,8 @@ describe Site do
     default_theme = Fabricate(:theme)
     SiteSetting.default_theme_id = default_theme.id
     user_theme = Fabricate(:theme, user_selectable: true)
+    second_user_theme = Fabricate(:theme, user_selectable: true)
+    color_scheme = Fabricate(:color_scheme)
 
     anon_guardian = Guardian.new
     user_guardian = Guardian.new(Fabricate(:user))
@@ -39,6 +41,11 @@ describe Site do
     expect_correct_themes(anon_guardian)
     expect_correct_themes(user_guardian)
 
+    second_user_theme.color_scheme_id = color_scheme.id
+    second_user_theme.save!
+
+    expect_correct_themes(anon_guardian)
+    expect_correct_themes(user_guardian)
   end
 
   it "returns correct notification level for categories" do