From 5604ce70d4b53a86c53d6109b4e5c896e1edb7d8 Mon Sep 17 00:00:00 2001 From: Penar Musaraj Date: Fri, 19 Feb 2021 11:22:24 -0500 Subject: [PATCH] DEV: More refactoring of SCSS importers (#12143) --- .../common/foundation/variables.scss | 1 - app/models/theme_field.rb | 12 ++--- lib/stylesheet/compiler.rb | 10 ++-- lib/stylesheet/importer.rb | 54 ++++++++++--------- spec/components/stylesheet/importer_spec.rb | 10 ++-- spec/components/stylesheet/manager_spec.rb | 24 +++++++++ 6 files changed, 68 insertions(+), 43 deletions(-) diff --git a/app/assets/stylesheets/common/foundation/variables.scss b/app/assets/stylesheets/common/foundation/variables.scss index 2712f8272bb..493c21c68a3 100644 --- a/app/assets/stylesheets/common/foundation/variables.scss +++ b/app/assets/stylesheets/common/foundation/variables.scss @@ -77,7 +77,6 @@ $line-height-large: 1.4; // Normal or small text // These files don't actually exist. They're injected by Stylesheet::Compiler. // -------------------------------------------------- -@import "theme_colors"; @import "plugins_variables"; @import "common/foundation/math"; diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index 2b47ddbcd5c..673386b791b 100644 --- a/app/models/theme_field.rb +++ b/app/models/theme_field.rb @@ -346,21 +346,19 @@ class ThemeField < ActiveRecord::Base end end - def compile_scss - scss = <<~SCSS - @import "common/foundation/variables"; @import "common/foundation/mixins"; #{self.theme.scss_variables.to_s} #{self.value} - SCSS + def compile_scss(prepended_scss = nil) + prepended_scss ||= Stylesheet::Importer.new({}).prepended_scss - Stylesheet::Compiler.compile(scss, + Stylesheet::Compiler.compile("#{prepended_scss} #{self.theme.scss_variables.to_s} #{self.value}", "#{Theme.targets[self.target_id]}.scss", theme: self.theme, load_paths: self.theme.scss_load_paths ) end - def compiled_css + def compiled_css(prepended_scss) css, _source_map = begin - compile_scss + compile_scss(prepended_scss) rescue SassC::SyntaxError => e # We don't want to raise a blocking error here # admin theme editor or discourse_theme CLI will show it nonetheless diff --git a/lib/stylesheet/compiler.rb b/lib/stylesheet/compiler.rb index c338f4a9918..5be3db27b77 100644 --- a/lib/stylesheet/compiler.rb +++ b/lib/stylesheet/compiler.rb @@ -9,12 +9,13 @@ module Stylesheet class Compiler def self.compile_asset(asset, options = {}) - file = "@import \"common/foundation/variables\"; @import \"common/foundation/mixins\";" + importer = Importer.new(options) + file = importer.prepended_scss if Importer::THEME_TARGETS.include?(asset.to_s) filename = "theme_#{options[:theme_id]}.scss" file += options[:theme_variables].to_s - file += Importer.new({ theme_id: options[:theme_id] }).theme_import(asset) + file += importer.theme_import(asset) elsif Importer.special_imports[asset.to_s] filename = "theme_#{options[:theme_id]}.scss" file += " @import \"#{asset}\";" @@ -24,13 +25,12 @@ module Stylesheet file += File.read path if asset.to_s == Stylesheet::Manager::COLOR_SCHEME_STYLESHEET - file += Stylesheet::Importer.import_color_definitions(options[:theme_id]) - file += Stylesheet::Importer.import_wcag_overrides(options[:color_scheme_id]) + file += importer.import_color_definitions + file += importer.import_wcag_overrides end end compile(file, filename, options) - end def self.compile(stylesheet, filename, options = {}) diff --git a/lib/stylesheet/importer.rb b/lib/stylesheet/importer.rb index 6f241f77d55..0a692c65c4f 100644 --- a/lib/stylesheet/importer.rb +++ b/lib/stylesheet/importer.rb @@ -95,25 +95,6 @@ module Stylesheet import_files(DiscoursePluginRegistry.sass_variables) end - register_import "theme_colors" do - contents = +"" - if @color_scheme_id - colors = begin - ColorScheme.find(@color_scheme_id).resolved_colors - rescue - ColorScheme.base_colors - end - else - colors = (@theme_id && theme.color_scheme) ? theme.color_scheme.resolved_colors : ColorScheme.base_colors - end - - colors.each do |n, hex| - contents << "$#{n}: ##{hex} !default;\n" - end - - Import.new("theme_colors.scss", source: contents) - end - register_import "category_backgrounds" do contents = +"" Category.where('uploaded_background_id IS NOT NULL').each do |c| @@ -127,7 +108,7 @@ module Stylesheet register_imports! - def self.import_color_definitions(theme_id) + def import_color_definitions contents = +"" DiscoursePluginRegistry.color_definition_stylesheets.each do |name, path| contents << "// Color definitions from #{name}\n\n" @@ -135,7 +116,7 @@ module Stylesheet contents << "\n\n" end - theme_id ||= SiteSetting.default_theme_id + theme_id = @theme_id || SiteSetting.default_theme_id resolved_ids = Theme.transform_ids([theme_id]) if resolved_ids @@ -147,7 +128,7 @@ module Stylesheet if field.theme_id == theme.id contents << field.value else - contents << field.compiled_css + contents << field.compiled_css(prepended_scss) end contents << "\n\n" end @@ -155,13 +136,36 @@ module Stylesheet contents end - def self.import_wcag_overrides(color_scheme_id) - if color_scheme_id && ColorScheme.find_by_id(color_scheme_id)&.is_wcag? + def import_wcag_overrides + if @color_scheme_id && ColorScheme.find_by_id(@color_scheme_id)&.is_wcag? return "@import \"wcag\";" end "" end + def color_variables + contents = +"" + if @color_scheme_id + colors = begin + ColorScheme.find(@color_scheme_id).resolved_colors + rescue + ColorScheme.base_colors + end + else + colors = (@theme_id && theme.color_scheme) ? theme.color_scheme.resolved_colors : ColorScheme.base_colors + end + + colors.each do |n, hex| + contents << "$#{n}: ##{hex} !default; " + end + + contents + end + + def prepended_scss + "#{color_variables} @import \"common/foundation/variables\"; @import \"common/foundation/mixins\"; " + end + def initialize(options) @theme = options[:theme] @theme_id = options[:theme_id] @@ -204,7 +208,7 @@ module Stylesheet if field.theme_id == theme.id contents << value else - contents << field.compiled_css + contents << field.compiled_css(prepended_scss) end end diff --git a/spec/components/stylesheet/importer_spec.rb b/spec/components/stylesheet/importer_spec.rb index 5787ae188f4..8f8375b9f89 100644 --- a/spec/components/stylesheet/importer_spec.rb +++ b/spec/components/stylesheet/importer_spec.rb @@ -80,7 +80,7 @@ describe Stylesheet::Importer do }} it "should include color definitions in the theme" do - styles = Stylesheet::Importer.import_color_definitions(theme.id) + styles = Stylesheet::Importer.new({ theme_id: theme.id }).import_color_definitions expect(styles).to include(scss) end @@ -88,14 +88,14 @@ describe Stylesheet::Importer do theme.add_relative_theme!(:child, child) theme.save! - styles = Stylesheet::Importer.import_color_definitions(theme.id) + styles = Stylesheet::Importer.new({ theme_id: theme.id }).import_color_definitions expect(styles).to include(scss_child) expect(styles).to include("Color definitions from Child Theme") end it "should include default theme color definitions" do SiteSetting.default_theme_id = theme.id - styles = Stylesheet::Importer.import_color_definitions(nil) + styles = Stylesheet::Importer.new({}).import_color_definitions expect(styles).to include(scss) end end @@ -103,12 +103,12 @@ describe Stylesheet::Importer do context "#import_wcag_overrides" do it "should do nothing on a regular scheme" do scheme = ColorScheme.create_from_base(name: 'Regular') - expect(Stylesheet::Importer.import_wcag_overrides(scheme.id)).to eq("") + expect(Stylesheet::Importer.new({ color_scheme_id: scheme.id }).import_wcag_overrides).to eq("") end it "should include WCAG overrides for WCAG based scheme" do scheme = ColorScheme.create_from_base(name: 'WCAG New', base_scheme_id: "WCAG Dark") - expect(Stylesheet::Importer.import_wcag_overrides(scheme.id)).to eq("@import \"wcag\";") + expect(Stylesheet::Importer.new({ color_scheme_id: scheme.id }).import_wcag_overrides).to eq("@import \"wcag\";") end end end diff --git a/spec/components/stylesheet/manager_spec.rb b/spec/components/stylesheet/manager_spec.rb index 5ed959d08f8..22b756de94d 100644 --- a/spec/components/stylesheet/manager_spec.rb +++ b/spec/components/stylesheet/manager_spec.rb @@ -288,14 +288,38 @@ describe Stylesheet::Manager do t.set_field(target: :common, name: "color_definitions", value: ':root {--special: rebeccapurple;}') t.save! }} + let(:scss_child) { ':root {--child-definition: #{dark-light-choose(#c00, #fff)};}' } + let(:child) { Fabricate(:theme, component: true, name: "Child Theme").tap { |t| + t.set_field(target: :common, name: "color_definitions", value: scss_child) + t.save! + }} let(:scheme) { ColorScheme.base } + let(:dark_scheme) { ColorScheme.create_from_base(name: 'Dark', base_scheme_id: 'Dark') } it "includes theme color definitions in color scheme" do stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, scheme).compile(force: true) expect(stylesheet).to include("--special: rebeccapurple") end + it "includes child color definitions in color schemes" do + theme.add_relative_theme!(:child, child) + theme.save! + stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, scheme).compile(force: true) + + expect(stylesheet).to include("--special: rebeccapurple") + expect(stylesheet).to include("--child-definition: #c00") + end + + it "respects selected color scheme in child color definitions" do + theme.add_relative_theme!(:child, child) + theme.save! + + stylesheet = Stylesheet::Manager.new(:color_definitions, theme.id, dark_scheme).compile(force: true) + expect(stylesheet).to include("--special: rebeccapurple") + expect(stylesheet).to include("--child-definition: #fff") + end + it "fails gracefully for broken SCSS" do scss = "$test: $missing-var;" theme.set_field(target: :common, name: "color_definitions", value: scss)