From 78a857931c1bfbc3d309ec6706fdfd3a612b68df Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 27 Jan 2025 11:29:55 +1000 Subject: [PATCH] FEATURE: Improve wizard font selection and set Inter as default font for new sites (#30974) This commit narrows down the list of fonts we offer in our setup wizard and simplifies things to only show a single font dropdown. This selection will then set the `base_font` and `heading_font` site setting to the same value. For existing sites that may have set different values, we will still show 2 dropdowns when visiting the wizard. We are also changing our default font to the more modern selection Inter, replacing Arial. Arial is very dependent on system installed fonts, whereas Inter we can package to everyone in Discourse. Finally, for existing sites that have not changed their default from Arial, we will keep that value via a migration so we do not surprise site owners with a completely new font. --- .../wizard/components/fields/dropdown.gjs | 6 +- .../fields/styling-preview/-preview-base.js | 9 +++ .../app/static/wizard/models/wizard.js | 10 ++- config/locales/server.en.yml | 2 + config/site_settings.yml | 10 ++- ...027_set_default_font_for_existing_sites.rb | 34 ++++++++++ lib/wizard/builder.rb | 67 +++++++++++++------ spec/lib/stylesheet/importer_spec.rb | 2 +- spec/lib/wizard/step_updater_spec.rb | 9 +++ spec/lib/wizard/wizard_builder_spec.rb | 11 ++- spec/system/page_objects/pages/wizard.rb | 13 ++++ spec/system/wizard_spec.rb | 28 ++++++-- 12 files changed, 167 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20250117065027_set_default_font_for_existing_sites.rb diff --git a/app/assets/javascripts/discourse/app/static/wizard/components/fields/dropdown.gjs b/app/assets/javascripts/discourse/app/static/wizard/components/fields/dropdown.gjs index b9e49306416..b6366671af0 100644 --- a/app/assets/javascripts/discourse/app/static/wizard/components/fields/dropdown.gjs +++ b/app/assets/javascripts/discourse/app/static/wizard/components/fields/dropdown.gjs @@ -20,7 +20,10 @@ export default class Dropdown extends Component { } } - if (this.args.field.id === "body_font") { + if ( + this.args.field.id === "body_font" || + this.args.field.id === "site_font" + ) { for (let choice of this.args.field.choices) { set(choice, "classNames", `body-font-${choice.id.replace(/_/g, "-")}`); } @@ -74,6 +77,7 @@ export default class Dropdown extends Component { return ColorPalettes; case "body_font": case "heading_font": + case "site_font": return FontSelector; case "homepage_style": return HomepageStyleSelector; diff --git a/app/assets/javascripts/discourse/app/static/wizard/components/fields/styling-preview/-preview-base.js b/app/assets/javascripts/discourse/app/static/wizard/components/fields/styling-preview/-preview-base.js index c6cb19e1e5a..abceb0424a3 100644 --- a/app/assets/javascripts/discourse/app/static/wizard/components/fields/styling-preview/-preview-base.js +++ b/app/assets/javascripts/discourse/app/static/wizard/components/fields/styling-preview/-preview-base.js @@ -57,6 +57,7 @@ export default class PreviewBase extends Component { this.step.findField("color_scheme")?.addListener(this.themeChanged); this.step.findField("homepage_style")?.addListener(this.themeChanged); this.step.findField("body_font")?.addListener(this.themeBodyFontChanged); + this.step.findField("site_font")?.addListener(this.themeFontChanged); this.step .findField("heading_font") ?.addListener(this.themeHeadingFontChanged); @@ -74,6 +75,7 @@ export default class PreviewBase extends Component { this.step .findField("body_font") ?.removeListener(this.themeBodyFontChanged); + this.step.findField("site_font")?.removeListener(this.themeFontChanged); this.step .findField("heading_font") ?.removeListener(this.themeHeadingFontChanged); @@ -99,6 +101,13 @@ export default class PreviewBase extends Component { } } + @action + themeFontChanged() { + if (!this.loadingFontVariants) { + this.loadFontVariants(this.wizard.font); + } + } + loadFontVariants(font) { if (!font) { return Promise.resolve(); diff --git a/app/assets/javascripts/discourse/app/static/wizard/models/wizard.js b/app/assets/javascripts/discourse/app/static/wizard/models/wizard.js index 5d8d9ea5883..73a60856ad6 100644 --- a/app/assets/javascripts/discourse/app/static/wizard/models/wizard.js +++ b/app/assets/javascripts/discourse/app/static/wizard/models/wizard.js @@ -50,11 +50,17 @@ export default class Wizard { } get font() { - return this.findStep("styling")?.findField("body_font").chosen; + const bodyFont = this.getFont("body"); + return bodyFont ? bodyFont : this.getFont("site"); } get headingFont() { - return this.findStep("styling")?.findField("heading_font").chosen; + const headingFont = this.getFont("heading"); + return headingFont ? headingFont : this.getFont("site"); + } + + getFont(type) { + return this.findStep("styling")?.findField(`${type}_font`)?.chosen; } findStep(id) { diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 1a85257d713..7ab8de8a275 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -5428,6 +5428,8 @@ en: label: "Color palette" body_font: label: "Body font" + site_font: + label: "Font" heading_font: label: "Heading font" styling_preview: diff --git a/config/site_settings.yml b/config/site_settings.yml index 60790e87f67..69c3cfc2d9d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -21,6 +21,12 @@ # defined in the choices option. A type:list setting with the word 'colors' in # its name will make color values have a bold line of the corresponding color # +# list_type: simple - Allow multiple values to be selected for the list, and new values can be defined if +# allow_any is true. This list displays like a list of items with add/remove buttons, +# and the ability to reorder items. +# list_type: compact - Allow multiple values to be selected for the list, and new values can be defined if +# allow_any is true. This list displays like the tag selector. +# # type: enum - A single value, chosen from a set of valid values in the choices option. # type: json_schema - The setting value is stored as JSON in the database. Used # in the format `json_schema: ClassName` where the class implements @@ -412,7 +418,7 @@ basic: default: "" hidden: true base_font: - default: "arial" + default: "inter" choices: "BaseFontSetting.values" refresh: true type: list @@ -420,7 +426,7 @@ basic: area: "fonts" allow_any: false heading_font: - default: "arial" + default: "inter" choices: "BaseFontSetting.values" refresh: true type: list diff --git a/db/migrate/20250117065027_set_default_font_for_existing_sites.rb b/db/migrate/20250117065027_set_default_font_for_existing_sites.rb new file mode 100644 index 00000000000..8ef3c6335e1 --- /dev/null +++ b/db/migrate/20250117065027_set_default_font_for_existing_sites.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class SetDefaultFontForExistingSites < ActiveRecord::Migration[7.2] + def up + return if !Migration::Helpers.existing_site? + + base_font_changed_from_default = + DB.query_single("SELECT 1 FROM site_settings WHERE name = 'base_font'").first == 1 + heading_font_changed_from_default = + DB.query_single("SELECT 1 FROM site_settings WHERE name = 'heading_font'").first == 1 + + if !base_font_changed_from_default + # Type 8 is 'list', arial is the current default font. + execute <<~SQL if Migration::Helpers.existing_site? + INSERT INTO site_settings(name, data_type, value, created_at, updated_at) + VALUES('base_font', 8, 'arial', NOW(), NOW()) + ON CONFLICT (name) DO NOTHING + SQL + end + + if !heading_font_changed_from_default + # Type 8 is 'list', arial is the current default font. + execute <<~SQL if Migration::Helpers.existing_site? + INSERT INTO site_settings(name, data_type, value, created_at, updated_at) + VALUES('heading_font', 8, 'arial', NOW(), NOW()) + ON CONFLICT (name) DO NOTHING + SQL + end + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/wizard/builder.rb b/lib/wizard/builder.rb index bd0b9d5c05d..fd98f6b7d7b 100644 --- a/lib/wizard/builder.rb +++ b/lib/wizard/builder.rb @@ -2,6 +2,8 @@ class Wizard class Builder + WIZARD_FONTS = %w[lato inter montserrat open_sans poppins roboto] + def initialize(user) @wizard = Wizard.new(user) end @@ -165,28 +167,50 @@ class Wizard themes.add_choice(t[:id], data: { colors: t[:colors] }) end - body_font = - step.add_field( - id: "body_font", - type: "dropdown", - value: SiteSetting.base_font, - show_in_sidebar: true, - ) + if SiteSetting.base_font != SiteSetting.heading_font + body_font = + step.add_field( + id: "body_font", + type: "dropdown", + value: SiteSetting.base_font, + show_in_sidebar: true, + ) - heading_font = - step.add_field( - id: "heading_font", - type: "dropdown", - value: SiteSetting.heading_font, - show_in_sidebar: true, - ) + heading_font = + step.add_field( + id: "heading_font", + type: "dropdown", + value: SiteSetting.heading_font, + show_in_sidebar: true, + ) + else + site_font = + step.add_field( + id: "site_font", + type: "dropdown", + value: SiteSetting.base_font, + show_in_sidebar: true, + ) + end + + allowed_fonts = WIZARD_FONTS + allowed_fonts << SiteSetting.base_font if !allowed_fonts.include?(SiteSetting.base_font) + if !allowed_fonts.include?(SiteSetting.heading_font) + allowed_fonts << SiteSetting.heading_font + end DiscourseFonts .fonts - .sort_by { |f| f[:name] } + .select do |font| + # We only want to display certain fonts in the wizard, others will be accessible + # in site settings. + allowed_fonts.include?(font[:key]) + end + .sort_by { |font| font[:name] } .each do |font| - body_font.add_choice(font[:key], label: font[:name]) - heading_font.add_choice(font[:key], label: font[:name]) + body_font&.add_choice(font[:key], label: font[:name]) + heading_font&.add_choice(font[:key], label: font[:name]) + site_font&.add_choice(font[:key], label: font[:name]) end current = @@ -217,8 +241,13 @@ class Wizard step.add_field(id: "styling_preview", type: "styling-preview") step.on_update do |updater| - updater.update_setting(:base_font, updater.fields[:body_font]) - updater.update_setting(:heading_font, updater.fields[:heading_font]) + if updater.fields[:site_font].present? + updater.update_setting(:base_font, updater.fields[:site_font]) + updater.update_setting(:heading_font, updater.fields[:site_font]) + else + updater.update_setting(:base_font, updater.fields[:body_font]) + updater.update_setting(:heading_font, updater.fields[:heading_font]) + end top_menu = SiteSetting.top_menu_map if !updater.fields[:homepage_style].include?("categories") && diff --git a/spec/lib/stylesheet/importer_spec.rb b/spec/lib/stylesheet/importer_spec.rb index 0f495a10c4d..53a0818c7b6 100644 --- a/spec/lib/stylesheet/importer_spec.rb +++ b/spec/lib/stylesheet/importer_spec.rb @@ -9,7 +9,7 @@ RSpec.describe Stylesheet::Importer do describe "#font" do it "includes font variable" do - default_font = ":root{--font-family: Arial, sans-serif}" + default_font = ":root{--font-family: Inter, Arial, sans-serif}" expect(compile_css("color_definitions")).to include(default_font) expect(compile_css("embed")).to include(default_font) expect(compile_css("publish")).to include(default_font) diff --git a/spec/lib/wizard/step_updater_spec.rb b/spec/lib/wizard/step_updater_spec.rb index 31dd897fb48..21c76f5b2ea 100644 --- a/spec/lib/wizard/step_updater_spec.rb +++ b/spec/lib/wizard/step_updater_spec.rb @@ -96,6 +96,15 @@ RSpec.describe Wizard::StepUpdater do expect(SiteSetting.heading_font).to eq("oswald") end + it "updates both fonts if site_font is used" do + updater = wizard.create_updater("styling", site_font: "open_sans", homepage_style: "latest") + updater.update + expect(updater.success?).to eq(true) + expect(wizard.completed_steps?("styling")).to eq(true) + expect(SiteSetting.base_font).to eq("open_sans") + expect(SiteSetting.heading_font).to eq("open_sans") + end + context "with colors" do context "with an existing color scheme" do fab!(:color_scheme) { Fabricate(:color_scheme, name: "existing", via_wizard: true) } diff --git a/spec/lib/wizard/wizard_builder_spec.rb b/spec/lib/wizard/wizard_builder_spec.rb index fff48241d90..7d790bc3e43 100644 --- a/spec/lib/wizard/wizard_builder_spec.rb +++ b/spec/lib/wizard/wizard_builder_spec.rb @@ -92,12 +92,19 @@ RSpec.describe Wizard::Builder do describe "styling" do let(:styling_step) { wizard.steps.find { |s| s.id == "styling" } } - let(:font_field) { styling_step.fields.find { |f| f.id == "body_font" } } + let(:font_field) { styling_step.fields.find { |f| f.id == "site_font" } } fab!(:theme) let(:colors_field) { styling_step.fields.first } + before do + SiteSetting.remove_override!(:base_font) + SiteSetting.remove_override!(:heading_font) + end + it "has the full list of available fonts in alphabetical order" do - expect(font_field.choices.map(&:label)).to eq(DiscourseFonts.fonts.map { |f| f[:name] }.sort) + expect(font_field.choices.map(&:label)).to eq( + ["Inter", "Lato", "Montserrat", "Open Sans", "Poppins", "Roboto"], + ) end context "with colors" do diff --git a/spec/system/page_objects/pages/wizard.rb b/spec/system/page_objects/pages/wizard.rb index 0cf4b0291b2..85749ed50b7 100644 --- a/spec/system/page_objects/pages/wizard.rb +++ b/spec/system/page_objects/pages/wizard.rb @@ -100,6 +100,13 @@ class PageObjects::Pages::Wizard::StylingStep < PageObjects::Pages::Wizard::Step select_kit.select_row_by_value(palette) end + def select_font_option(font) + select_kit = + PageObjects::Components::SelectKit.new(".dropdown-site-font .wizard-container__dropdown") + select_kit.expand + select_kit.select_row_by_value(font) + end + def select_body_font_option(font) select_kit = PageObjects::Components::SelectKit.new(".dropdown-body-font .wizard-container__dropdown") @@ -127,6 +134,12 @@ class PageObjects::Pages::Wizard::StylingStep < PageObjects::Pages::Wizard::Step select_kit.has_selected_value?(palette) end + def has_selected_font?(font) + select_kit = + PageObjects::Components::SelectKit.new(".dropdown-site-font .wizard-container__dropdown") + select_kit.has_selected_value?(font) + end + def has_selected_body_font?(font) select_kit = PageObjects::Components::SelectKit.new(".dropdown-body-font .wizard-container__dropdown") diff --git a/spec/system/wizard_spec.rb b/spec/system/wizard_spec.rb index 0374acdcb55..ba1aaee3bbe 100644 --- a/spec/system/wizard_spec.rb +++ b/spec/system/wizard_spec.rb @@ -82,13 +82,12 @@ describe "Wizard", type: :system do end describe "Wizard Step: Styling" do - it "lets user configure styling including fonts and colors" do + it "lets user configure styling including font and colors" do wizard_page.go_to_step("styling") expect(wizard_page).to be_on_step("styling") wizard_page.styling_step.select_color_palette_option("Dark") - wizard_page.styling_step.select_body_font_option("lato") - wizard_page.styling_step.select_heading_font_option("merriweather") + wizard_page.styling_step.select_font_option("roboto") wizard_page.styling_step.select_homepage_style_option("hot") wizard_page.go_to_next_step @@ -97,17 +96,32 @@ describe "Wizard", type: :system do expect(Theme.find_default.color_scheme_id).to eq( ColorScheme.find_by(base_scheme_id: "Dark", via_wizard: true).id, ) - expect(SiteSetting.base_font).to eq("lato") - expect(SiteSetting.heading_font).to eq("merriweather") + expect(SiteSetting.base_font).to eq("roboto") + expect(SiteSetting.heading_font).to eq("roboto") expect(SiteSetting.homepage).to eq("hot") wizard_page.go_to_step("styling") expect(wizard_page.styling_step).to have_selected_color_palette("Dark") - expect(wizard_page.styling_step).to have_selected_body_font("lato") - expect(wizard_page.styling_step).to have_selected_heading_font("merriweather") + expect(wizard_page.styling_step).to have_selected_font("roboto") expect(wizard_page.styling_step).to have_selected_homepage_style("hot") end + + it "lets user select separate body and heading font if they are already seperate" do + SiteSetting.base_font = "poppins" + SiteSetting.heading_font = "montserrat" + wizard_page.go_to_step("styling") + expect(wizard_page).to be_on_step("styling") + + wizard_page.styling_step.select_body_font_option("roboto") + wizard_page.styling_step.select_heading_font_option("inter") + + wizard_page.go_to_next_step + expect(wizard_page).to be_on_step("ready") + + expect(SiteSetting.base_font).to eq("roboto") + expect(SiteSetting.heading_font).to eq("inter") + end end describe "Wizard Step: Ready" do