From 10f34ddf86a43e08fd30dc325d4d7ffefcdf832e Mon Sep 17 00:00:00 2001 From: Osama Sayegh Date: Thu, 23 Jan 2025 15:54:49 +0300 Subject: [PATCH] DEV: Backend support for light/dark mode in color palettes (#30893) We're embarking on a project for overhauling the color palette and theme systems in Discourse. As part of this project, we're making each color palette include light and dark modes instead of the status quo of requiring 2 separate color palettes to implement light and dark modes. This commit is a first step towards that goal; it adds a code path for generating and serving `color_definitions` stylesheets using the built-in dark variant of a color palette. All of this code path is behind a default-off site setting `use_overhauled_theme_color_palette`, so there's no change in behavior unless the setting is enabled. Internal topic: t/141467. --- app/helpers/application_helper.rb | 12 ++- app/models/color_scheme.rb | 24 ++++-- app/models/color_scheme_color.rb | 2 + config/site_settings.yml | 3 + ...5539_add_dark_hex_to_color_scheme_color.rb | 7 ++ lib/stylesheet/importer.rb | 11 ++- lib/stylesheet/manager.rb | 44 ++++++----- lib/stylesheet/manager/builder.rb | 12 ++- spec/helpers/application_helper_spec.rb | 46 +++++++++++- spec/lib/stylesheet/manager_spec.rb | 73 ++++++++++++++++++- 10 files changed, 193 insertions(+), 41 deletions(-) create mode 100644 db/migrate/20250120115539_add_dark_hex_to_color_scheme_color.rb diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 0296b676eb5..d10380f0d1c 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -555,8 +555,12 @@ module ApplicationHelper end def dark_scheme_id - cookies[:dark_scheme_id] || current_user&.user_option&.dark_scheme_id || - SiteSetting.default_dark_mode_color_scheme_id + if SiteSetting.use_overhauled_theme_color_palette + scheme_id + else + cookies[:dark_scheme_id] || current_user&.user_option&.dark_scheme_id || + SiteSetting.default_dark_mode_color_scheme_id + end end def current_homepage @@ -638,6 +642,7 @@ module ApplicationHelper result << stylesheet_manager.color_scheme_stylesheet_preload_tag( dark_scheme_id, "(prefers-color-scheme: dark)", + dark: SiteSetting.use_overhauled_theme_color_palette, ) end @@ -657,6 +662,7 @@ module ApplicationHelper dark_scheme_id, "(prefers-color-scheme: dark)", self.method(:add_resource_preload_list), + dark: SiteSetting.use_overhauled_theme_color_palette, ) end @@ -668,7 +674,7 @@ module ApplicationHelper if dark_scheme_id != -1 result << <<~HTML - + HTML else result << <<~HTML diff --git a/app/models/color_scheme.rb b/app/models/color_scheme.rb index 30d3f97ce0c..921762957e4 100644 --- a/app/models/color_scheme.rb +++ b/app/models/color_scheme.rb @@ -409,16 +409,18 @@ class ColorScheme < ActiveRecord::Base new_color_scheme end - def self.lookup_hex_for_name(name, scheme_id = nil) + def self.lookup_hex_for_name(name, scheme_id = nil, dark: false) enabled_color_scheme = find_by(id: scheme_id) if scheme_id enabled_color_scheme ||= Theme.where(id: SiteSetting.default_theme_id).first&.color_scheme - (enabled_color_scheme || base).colors.find { |c| c.name == name }.try(:hex) + color_record = (enabled_color_scheme || base).colors.find { |c| c.name == name } + return if !color_record + dark ? color_record.dark_hex || color_record.hex : color_record.hex end - def self.hex_for_name(name, scheme_id = nil) - hex_cache.defer_get_set(scheme_id ? name + "_#{scheme_id}" : name) do - lookup_hex_for_name(name, scheme_id) - end + def self.hex_for_name(name, scheme_id = nil, dark: false) + cache_key = scheme_id ? "#{name}_#{scheme_id}" : name + cache_key += "_dark" if dark + hex_cache.defer_get_set(cache_key) { lookup_hex_for_name(name, scheme_id, dark:) } end def colors=(arr) @@ -450,10 +452,16 @@ class ColorScheme < ActiveRecord::Base colors || ColorScheme.base_colors end - def resolved_colors + def resolved_colors(dark: false) from_base = ColorScheme.base_colors from_custom_scheme = base_colors - from_db = colors.map { |c| [c.name, c.hex] }.to_h + from_db = + colors + .map do |c| + hex = dark ? (c.dark_hex || c.hex) : c.hex + [c.name, hex] + end + .to_h resolved = from_base.merge(from_custom_scheme).except("hover", "selected").merge(from_db) diff --git a/app/models/color_scheme_color.rb b/app/models/color_scheme_color.rb index aa24a13bf5c..ce74d38ee26 100644 --- a/app/models/color_scheme_color.rb +++ b/app/models/color_scheme_color.rb @@ -4,6 +4,7 @@ class ColorSchemeColor < ActiveRecord::Base belongs_to :color_scheme validates :hex, format: { with: /\A([0-9a-fA-F]{3}|[0-9a-fA-F]{6})\z/ } + validates :dark_hex, format: { with: /\A([0-9a-fA-F]{3}|[0-9a-fA-F]{6})\z/ }, allow_nil: true def hex_with_hash "##{hex}" @@ -20,6 +21,7 @@ end # color_scheme_id :integer not null # created_at :datetime not null # updated_at :datetime not null +# dark_hex :string(6) # # Indexes # diff --git a/config/site_settings.yml b/config/site_settings.yml index f636835e720..a8904196ebe 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -3522,3 +3522,6 @@ experimental: type: group_list list_type: compact area: "group_permissions" + use_overhauled_theme_color_palette: + default: false + hidden: true diff --git a/db/migrate/20250120115539_add_dark_hex_to_color_scheme_color.rb b/db/migrate/20250120115539_add_dark_hex_to_color_scheme_color.rb new file mode 100644 index 00000000000..cad3f7ef462 --- /dev/null +++ b/db/migrate/20250120115539_add_dark_hex_to_color_scheme_color.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddDarkHexToColorSchemeColor < ActiveRecord::Migration[7.2] + def change + add_column :color_scheme_colors, :dark_hex, :string, limit: 6 + end +end diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb index 5d53844b295..601bfbe12d8 100644 --- a/lib/stylesheet/importer.rb +++ b/lib/stylesheet/importer.rb @@ -145,19 +145,21 @@ module Stylesheet if @color_scheme_id colors = begin - ColorScheme.find(@color_scheme_id).resolved_colors + ColorScheme.find(@color_scheme_id).resolved_colors(dark: @dark) rescue StandardError ColorScheme.base_colors end elsif (@theme_id && !theme.component) - colors = theme&.color_scheme&.resolved_colors || ColorScheme.base_colors + colors = theme&.color_scheme&.resolved_colors(dark: @dark) || ColorScheme.base_colors else # this is a slightly ugly backwards compatibility fix, # we shouldn't be using the default theme color scheme for components # (most components use CSS custom properties which work fine without this) colors = - Theme.find_by_id(SiteSetting.default_theme_id)&.color_scheme&.resolved_colors || - ColorScheme.base_colors + Theme + .find_by_id(SiteSetting.default_theme_id) + &.color_scheme + &.resolved_colors(dark: @dark) || ColorScheme.base_colors end colors.each { |n, hex| contents << "$#{n}: ##{hex} !default; " } @@ -178,6 +180,7 @@ module Stylesheet @theme = options[:theme] @theme_id = options[:theme_id] @color_scheme_id = options[:color_scheme_id] + @dark = options[:dark] if @theme && !@theme_id # make up an id so other stuff does not bail out diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb index ce66bd44d38..0c2c0b107ec 100644 --- a/lib/stylesheet/manager.rb +++ b/lib/stylesheet/manager.rb @@ -42,10 +42,11 @@ class Stylesheet::Manager cache.clear_regex(/#{plugin}/) end - def self.color_scheme_cache_key(color_scheme, theme_id = nil) + def self.color_scheme_cache_key(color_scheme, theme_id = nil, dark: false) color_scheme_name = Slug.for(color_scheme.name) + color_scheme&.id.to_s theme_string = theme_id ? "_theme#{theme_id}" : "" - "#{COLOR_SCHEME_STYLESHEET}_#{color_scheme_name}_#{theme_string}_#{Discourse.current_hostname}" + dark_string = dark ? "_dark" : "" + "#{COLOR_SCHEME_STYLESHEET}_#{color_scheme_name}_#{theme_string}_#{Discourse.current_hostname}#{dark_string}" end def self.precompile_css @@ -114,14 +115,17 @@ class Stylesheet::Manager theme = manager.get_theme(theme_id) [theme_color_scheme, *color_schemes].compact.uniq.each do |scheme| - $stderr.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} #{theme.name} (#{scheme.name})" - - Stylesheet::Manager::Builder.new( - target: COLOR_SCHEME_STYLESHEET, - theme: theme, - color_scheme: scheme, - manager: manager, - ).compile(force: true) + [true, false].each do |dark| + mode = dark ? "dark" : "light" + $stderr.puts "precompile target: #{COLOR_SCHEME_STYLESHEET} #{theme.name} (#{scheme.name}) (#{mode})" + Stylesheet::Manager::Builder.new( + target: COLOR_SCHEME_STYLESHEET, + theme: theme, + color_scheme: scheme, + manager: manager, + dark:, + ).compile(force: true) + end end clear_color_scheme_cache! @@ -336,7 +340,7 @@ class Stylesheet::Manager end end - def color_scheme_stylesheet_details(color_scheme_id = nil, media) + def color_scheme_stylesheet_details(color_scheme_id = nil, media, dark: false) theme_id = @theme_id || SiteSetting.default_theme_id color_scheme = @@ -353,10 +357,10 @@ class Stylesheet::Manager target = COLOR_SCHEME_STYLESHEET.to_sym current_hostname = Discourse.current_hostname - cache_key = self.class.color_scheme_cache_key(color_scheme, theme_id) + cache_key = self.class.color_scheme_cache_key(color_scheme, theme_id, dark:) cache.defer_get_set(cache_key) do - stylesheet = { color_scheme_id: color_scheme.id } + stylesheet = { color_scheme_id: color_scheme.id, dark: } theme = get_theme(theme_id) @@ -366,6 +370,7 @@ class Stylesheet::Manager theme: get_theme(theme_id), color_scheme: color_scheme, manager: self, + dark:, ) builder.compile unless File.exist?(builder.stylesheet_fullpath) @@ -376,8 +381,8 @@ class Stylesheet::Manager end end - def color_scheme_stylesheet_preload_tag(color_scheme_id = nil, media = "all") - stylesheet = color_scheme_stylesheet_details(color_scheme_id, media) + def color_scheme_stylesheet_preload_tag(color_scheme_id = nil, media = "all", dark: false) + stylesheet = color_scheme_stylesheet_details(color_scheme_id, media, dark:) return "" if !stylesheet @@ -386,8 +391,13 @@ class Stylesheet::Manager %[].html_safe end - def color_scheme_stylesheet_link_tag(color_scheme_id = nil, media = "all", preload_callback = nil) - stylesheet = color_scheme_stylesheet_details(color_scheme_id, media) + def color_scheme_stylesheet_link_tag( + color_scheme_id = nil, + media = "all", + preload_callback = nil, + dark: false + ) + stylesheet = color_scheme_stylesheet_details(color_scheme_id, media, dark:) return "" if !stylesheet diff --git a/lib/stylesheet/manager/builder.rb b/lib/stylesheet/manager/builder.rb index 738770de3ce..005dabb11ae 100644 --- a/lib/stylesheet/manager/builder.rb +++ b/lib/stylesheet/manager/builder.rb @@ -3,11 +3,12 @@ class Stylesheet::Manager::Builder attr_reader :theme - def initialize(target: :desktop, theme: nil, color_scheme: nil, manager:) + def initialize(target: :desktop, theme: nil, color_scheme: nil, manager:, dark: false) @target = target @theme = theme @color_scheme = color_scheme @manager = manager + @dark = dark end def compile(opts = {}) @@ -46,6 +47,7 @@ class Stylesheet::Manager::Builder source_map_file: source_map_url_relative_from_stylesheet, color_scheme_id: @color_scheme&.id, load_paths: load_paths, + dark: @dark, ) rescue SassC::SyntaxError, SassC::NotRenderedError => e if Stylesheet::Importer::THEME_TARGETS.include?(@target.to_s) @@ -119,13 +121,14 @@ class Stylesheet::Manager::Builder end def qualified_target + dark_string = @dark ? "_dark" : "" if is_theme? "#{@target}_#{theme&.id}" elsif @color_scheme - "#{@target}_#{scheme_slug}_#{@color_scheme&.id}_#{@theme&.id}" + "#{@target}_#{scheme_slug}_#{@color_scheme&.id}_#{@theme&.id}#{dark_string}" else scheme_string = theme&.color_scheme ? "_#{theme.color_scheme.id}" : "" - "#{@target}#{scheme_string}" + "#{@target}#{scheme_string}#{dark_string}" end end @@ -245,8 +248,9 @@ class Stylesheet::Manager::Builder digest_string = "#{current_hostname}-" if cs theme_color_defs = resolve_baked_field(:common, :color_definitions) + dark_string = @dark ? "-dark" : "" digest_string += - "#{RailsMultisite::ConnectionManagement.current_db}-#{cs&.id}-#{cs&.version}-#{theme_color_defs}-#{Stylesheet::Manager.fs_asset_cachebuster}-#{fonts}" + "#{RailsMultisite::ConnectionManagement.current_db}-#{cs&.id}-#{cs&.version}-#{theme_color_defs}-#{Stylesheet::Manager.fs_asset_cachebuster}-#{fonts}#{dark_string}" else digest_string += "defaults-#{Stylesheet::Manager.fs_asset_cachebuster}-#{fonts}" diff --git a/spec/helpers/application_helper_spec.rb b/spec/helpers/application_helper_spec.rb index 568e0248986..89e1e8de879 100644 --- a/spec/helpers/application_helper_spec.rb +++ b/spec/helpers/application_helper_spec.rb @@ -877,12 +877,20 @@ RSpec.describe ApplicationHelper do describe "#discourse_theme_color_meta_tags" do before do light = Fabricate(:color_scheme) - light.color_scheme_colors << ColorSchemeColor.new(name: "header_background", hex: "abcdef") + light.color_scheme_colors << ColorSchemeColor.new( + name: "header_background", + hex: "abcdef", + dark_hex: "fedcba", + ) light.save! helper.request.cookies["color_scheme_id"] = light.id dark = Fabricate(:color_scheme) - dark.color_scheme_colors << ColorSchemeColor.new(name: "header_background", hex: "defabc") + dark.color_scheme_colors << ColorSchemeColor.new( + name: "header_background", + hex: "defabc", + dark_hex: "cbafed", + ) dark.save! helper.request.cookies["dark_scheme_id"] = dark.id end @@ -902,6 +910,17 @@ RSpec.describe ApplicationHelper do HTML end + + context "when use_overhauled_theme_color_palette setting is true" do + before { SiteSetting.use_overhauled_theme_color_palette = true } + + it "renders a light and dark theme-color meta tag using the light and dark palettes of the same color scheme record" do + expect(helper.discourse_theme_color_meta_tags).to eq(<<~HTML) + + + HTML + end + end end describe "#discourse_color_scheme_meta_tag" do @@ -944,4 +963,27 @@ RSpec.describe ApplicationHelper do HTML end end + + describe "#dark_scheme_id" do + fab!(:dark_scheme) { Fabricate(:color_scheme) } + fab!(:light_scheme) { Fabricate(:color_scheme) } + + before do + helper.request.cookies["color_scheme_id"] = light_scheme.id + helper.request.cookies["dark_scheme_id"] = dark_scheme.id + end + + it "returns the value set in the dark_scheme_id cookie" do + expect(helper.dark_scheme_id).to eq(dark_scheme.id) + end + + context "when use_overhauled_theme_color_palette is true" do + before { SiteSetting.use_overhauled_theme_color_palette = true } + + it "returns the same value as #scheme_id" do + expect(helper.dark_scheme_id).to eq(helper.scheme_id) + expect(helper.scheme_id).to eq(light_scheme.id) + end + end + end end diff --git a/spec/lib/stylesheet/manager_spec.rb b/spec/lib/stylesheet/manager_spec.rb index b779c02b5d7..ca3359cdbbc 100644 --- a/spec/lib/stylesheet/manager_spec.rb +++ b/spec/lib/stylesheet/manager_spec.rb @@ -676,6 +676,73 @@ RSpec.describe Stylesheet::Manager do expect(link).to include("/stylesheets/color_definitions_funky-bunch_#{cs.id}_") end + it "generates the dark mode of a color scheme when the dark option is specified" do + scheme = ColorScheme.create_from_base(name: "Neutral", base_scheme_id: "Neutral") + ColorSchemeRevisor.revise( + scheme, + colors: [{ name: "primary", hex: "CABFAF", dark_hex: "FAFCAB" }], + ) + theme = Fabricate(:theme) + manager = manager(theme.id) + + dark_stylesheet = + Stylesheet::Manager::Builder.new( + target: :color_definitions, + theme: theme, + color_scheme: scheme, + manager: manager, + dark: true, + ).compile + light_stylesheet = + Stylesheet::Manager::Builder.new( + target: :color_definitions, + theme: theme, + color_scheme: scheme, + manager: manager, + ).compile + + expect(light_stylesheet).to include("--primary: #CABFAF;") + expect(light_stylesheet).to include("color_definitions_neutral_#{scheme.id}_#{theme.id}") + expect(light_stylesheet).not_to include( + "color_definitions_neutral_#{scheme.id}_#{theme.id}_dark", + ) + + expect(dark_stylesheet).to include("--primary: #FAFCAB;") + expect(dark_stylesheet).to include("color_definitions_neutral_#{scheme.id}_#{theme.id}_dark") + end + + it "uses the light colors as fallback if the dark scheme doesn't define them" do + scheme = ColorScheme.create_from_base(name: "Neutral", base_scheme_id: "Neutral") + ColorSchemeRevisor.revise(scheme, colors: [{ name: "primary", hex: "BACFAB", dark_hex: nil }]) + theme = Fabricate(:theme) + manager = manager(theme.id) + + dark_stylesheet = + Stylesheet::Manager::Builder.new( + target: :color_definitions, + theme: theme, + color_scheme: scheme, + manager: manager, + dark: true, + ).compile + light_stylesheet = + Stylesheet::Manager::Builder.new( + target: :color_definitions, + theme: theme, + color_scheme: scheme, + manager: manager, + ).compile + + expect(light_stylesheet).to include("--primary: #BACFAB;") + expect(light_stylesheet).to include("color_definitions_neutral_#{scheme.id}_#{theme.id}") + expect(light_stylesheet).not_to include( + "color_definitions_neutral_#{scheme.id}_#{theme.id}_dark", + ) + + expect(dark_stylesheet).to include("--primary: #BACFAB;") + expect(dark_stylesheet).to include("color_definitions_neutral_#{scheme.id}_#{theme.id}_dark") + end + it "updates outputted colors when updating a color scheme" do scheme = ColorScheme.create_from_base(name: "Neutral", base_scheme_id: "Neutral") theme = Fabricate(:theme) @@ -905,7 +972,7 @@ RSpec.describe Stylesheet::Manager do # Ensure we force compile each theme only once expect(output.scan(/#{child_theme_with_css.name}/).length).to eq(2) - expect(StylesheetCache.count).to eq(22) # (3 themes * 2 targets) + 16 color schemes (2 themes * 8 color schemes (7 defaults + 1 theme scheme)) + expect(StylesheetCache.count).to eq(38) # (3 themes * 2 targets) + 32 color schemes (2 themes * 8 color schemes (7 defaults + 1 theme scheme) * 2 (light and dark mode per scheme)) end it "generates precompiled CSS - core and themes" do @@ -913,7 +980,7 @@ RSpec.describe Stylesheet::Manager do Stylesheet::Manager.precompile_theme_css results = StylesheetCache.pluck(:target) - expect(results.size).to eq(30) # 11 core targets + 9 theme + 10 color schemes + expect(results.size).to eq(46) # 8 core targets + 6 theme + 32 color schemes (light and dark mode per scheme) theme_targets.each do |tar| expect( @@ -929,7 +996,7 @@ RSpec.describe Stylesheet::Manager do Stylesheet::Manager.precompile_theme_css results = StylesheetCache.pluck(:target) - expect(results.size).to eq(30) # 11 core targets + 9 theme + 10 color schemes + expect(results.size).to eq(46) # 8 core targets + 6 theme + 32 color schemes (light and dark mode per scheme) expect(results).to include("color_definitions_#{scheme1.name}_#{scheme1.id}_#{user_theme.id}") expect(results).to include(