DEV: Improve theme support for color definitions (#10634)

DEV: Improve theme support for color definitions (#10634)

  • Lets child components extend color definitions
  • Includes default theme color definitions
  • Fails gracefully on color stylesheet SCSS errors
  • Includes theme variables when extending colors
diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb
index 1e74116..b59a097 100644
--- a/lib/stylesheet/importer.rb
+++ b/lib/stylesheet/importer.rb
@@ -152,8 +152,12 @@ module Stylesheet
         contents << "\n\n"
       end
 
-      if theme_id
-        Theme.list_baked_fields([theme_id], :common, :color_definitions).each do |row|
+      theme_id ||= SiteSetting.default_theme_id
+      resolved_ids = Theme.transform_ids([theme_id])
+
+      if resolved_ids
+        contents << " @import \"theme_variables\";"
+        Theme.list_baked_fields(resolved_ids, :common, :color_definitions).each do |row|
           contents << "// Color definitions from #{Theme.find_by_id(theme_id)&.name}\n\n"
           contents << row.value
         end
diff --git a/lib/stylesheet/manager.rb b/lib/stylesheet/manager.rb
index 3332b7e..847de64 100644
--- a/lib/stylesheet/manager.rb
+++ b/lib/stylesheet/manager.rb
@@ -244,6 +244,10 @@ class Stylesheet::Manager
       if Stylesheet::Importer::THEME_TARGETS.include?(@target.to_s)
         # no special errors for theme, handled in theme editor
         ["", nil]
+      elsif @target.to_s == COLOR_SCHEME_STYLESHEET
+        # log error but do not crash for errors in color definitions SCSS
+        Rails.logger.error "SCSS compilation error: #{e.message}"
+        ["", nil]
       else
         raise Discourse::ScssError, e.message
       end
diff --git a/spec/components/stylesheet/importer_spec.rb b/spec/components/stylesheet/importer_spec.rb
index 9c95d86..e3a6312 100644
--- a/spec/components/stylesheet/importer_spec.rb
+++ b/spec/components/stylesheet/importer_spec.rb
@@ -138,4 +138,40 @@ describe Stylesheet::Importer do
 
   end
 
+  context "#import_color_definitions" do
+    let(:scss) { ":root { --custom-color: green}" }
+    let(:scss_child) { ":root { --custom-color: red}" }
+
+    let(:theme) do
+      Fabricate(:theme).tap do |t|
+        t.set_field(target: :common, name: "color_definitions", value: scss)
+        t.save!
+      end
+    end
+
+    let(:child) { Fabricate(:theme, component: true).tap { |t|
+      t.set_field(target: :common, name: "color_definitions", value: scss_child)
+      t.save!
+    }}
+
+    it "should include color definitions in the theme" do
+      styles = Stylesheet::Importer.import_color_definitions(theme.id)
+      expect(styles).to include(scss)
+    end
+
+    it "should include color definitions from components" do
+      theme.add_relative_theme!(:child, child)
+      theme.save!
+
+      styles = Stylesheet::Importer.import_color_definitions(theme.id)
+      expect(styles).to include(scss_child)
+    end
+
+    it "should include default theme color definitions" do
+      SiteSetting.default_theme_id = theme.id
+      styles = Stylesheet::Importer.import_color_definitions(nil)
+      expect(styles).to include(scss)
+    end
+
+  end
 end
diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb
index 2ae8699..1a6f493 100644
--- a/spec/components/stylesheet/manager_spec.rb
+++ b/spec/components/stylesheet/manager_spec.rb
@@ -263,15 +263,28 @@ describe Stylesheet::Manager do
       expect(stylesheet2).to include("--primary: #c00;")
     end
 
-    it "includes theme color definitions in color scheme" do
-      theme = Fabricate(:theme)
-      theme.set_field(target: :common, name: :color_definitions, value: ':root {--special: rebeccapurple;}')
-      theme.save!
+    context "theme colors" do
+      let(:theme) { Fabricate(:theme).tap { |t|
+        t.set_field(target: :common, name: "color_definitions", value: ':root {--special: rebeccapurple;}')
+        t.save!
+      }}
 
-      scheme = ColorScheme.base
-      stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, scheme).compile
+      let(:scheme) { ColorScheme.base }
+
+      it "includes theme color definitions in color scheme" do
+        stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, scheme).compile
+        expect(stylesheet).to include("--special: rebeccapurple")
+      end
+
+      it "fails gracefully for broken SCSS" do
+        scss = "$test: $missing-var;"
+        theme.set_field(target: :common, name: "color_definitions", value: scss)
+        theme.save!
 
-      expect(stylesheet).to include("--special: rebeccapurple")
+        stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, scheme)
+
+        expect { stylesheet.compile }.not_to raise_error
+      end
     end
 
     context 'encoded slugs' do

GitHub sha: b4099543

1 Like

This commit appears in #10634 which was approved by eviltrout. It was merged by pmusaraj.