From c819284660d143be754ca07c62ff76ff55839c29 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Thu, 7 Jan 2021 11:15:38 -0500 Subject: [PATCH] UX: Improve color scheme choices in user prefs (#11656) --- .../app/controllers/preferences/interface.js | 36 ++++++-- .../app/templates/preferences/interface.hbs | 16 ++-- .../user-preferences-interface-test.js | 88 +++++++++++++++---- app/models/theme.rb | 2 +- config/locales/client.en.yml | 2 +- spec/models/site_spec.rb | 7 ++ 6 files changed, 118 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js b/app/assets/javascripts/discourse/app/controllers/preferences/interface.js index 74f0940d3ff..aff0e805b8a 100644 --- a/app/assets/javascripts/discourse/app/controllers/preferences/interface.js +++ b/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({ - selectedColorSchemeId: this.session.userColorSchemeId, - selectedDarkColorSchemeId: this.session.userDarkSchemeId, - }); + this.set("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); diff --git a/app/assets/javascripts/discourse/app/templates/preferences/interface.hbs b/app/assets/javascripts/discourse/app/templates/preferences/interface.hbs index d51e6de26fc..2afd22045a5 100644 --- a/app/assets/javascripts/discourse/app/templates/preferences/interface.hbs +++ b/app/assets/javascripts/discourse/app/templates/preferences/interface.hbs @@ -29,14 +29,14 @@ {{/if}}
{{combo-box - content=userSelectableColorSchemes - value=selectedColorSchemeId - onChange=(action "loadColorScheme") - options=(hash - translatedNone=selectedColorSchemeNoneLabel - ) - - }} + content=userSelectableColorSchemes + value=selectedColorSchemeId + onChange=(action "loadColorScheme") + options=(hash + translatedNone=selectedColorSchemeNoneLabel + autoInsertNoneItem=showColorSchemeNoneItem + ) + }}
{{#if showDarkColorSchemeSelector}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-interface-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-interface-test.js index 8f1de7289f9..4762fad7510 100644 --- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-interface-test.js +++ b/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($(".dark-color-scheme").length, "has dark color scheme dropdown"); + assert.ok(exists(".light-color-scheme"), "has regular 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.equal( - $(".dark-color-scheme").length, - 0, + assert.ok(exists(".light-color-scheme"), "has regular picker dropdown"); + assert.ok( + !exists(".dark-color-scheme"), "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( - $(".dark-color-scheme").length, - "has dark color scheme dropdown" - ); + assert.ok(exists(".light-color-scheme"), "has regular dropdown"); + assert.ok(exists(".dark-color-scheme"), "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( - $(".control-group.dark-mode").length, + assert.ok( + !exists(".control-group.dark-mode"), 0, "it does not show disable dark mode checkbox" ); diff --git a/app/models/theme.rb b/app/models/theme.rb index 57917e0236d..d6c4387bd27 100644 --- a/app/models/theme.rb +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 53351c79705..74ecc13e69a 100644 --- a/config/locales/client.en.yml +++ b/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" diff --git a/spec/models/site_spec.rb b/spec/models/site_spec.rb index cf8a8ba9cc5..78b3faec630 100644 --- a/spec/models/site_spec.rb +++ b/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