From 7e304e0a12115c6596fc64ff173368cba1a28e7b Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Wed, 24 Apr 2024 13:20:54 +0800 Subject: [PATCH] DEV: Migrate icons and links settings to new objects setting type (#36) Since https://github.com/discourse/discourse/commit/a440e15291e98a866f80a6566d5ec0597ab77e2e, we have started to support objects typed theme setting so we are switching this theme component to use it instead as it provides a much better UX for configuring the settings required for the theme component. --- .discourse-compatibility | 1 + .../components/brand-header-contents.gjs | 79 +++++++++++++++++++ .../components/brand-header-contents.hbs | 44 ----------- .../components/brand-header-contents.js | 44 ----------- locales/en.yml | 29 +++++++ .../settings/0001-migrate-links-setting.js | 31 ++++++++ .../settings/0002-migrate-icons-setting.js | 31 ++++++++ settings.yaml | 63 +++++++++++++-- spec/system/viewing_brand_header_spec.rb | 19 +++-- .../0001-migrate-links-setting-test.js | 42 ++++++++++ .../0002-migrate-icons-setting-test.js | 42 ++++++++++ 11 files changed, 324 insertions(+), 101 deletions(-) create mode 100644 javascripts/discourse/components/brand-header-contents.gjs delete mode 100644 javascripts/discourse/components/brand-header-contents.hbs delete mode 100644 javascripts/discourse/components/brand-header-contents.js create mode 100644 locales/en.yml create mode 100644 migrations/settings/0001-migrate-links-setting.js create mode 100644 migrations/settings/0002-migrate-icons-setting.js create mode 100644 test/unit/migrations/settings/0001-migrate-links-setting-test.js create mode 100644 test/unit/migrations/settings/0002-migrate-icons-setting-test.js diff --git a/.discourse-compatibility b/.discourse-compatibility index f280255..69bd4a1 100644 --- a/.discourse-compatibility +++ b/.discourse-compatibility @@ -1,3 +1,4 @@ +< 3.3.0.beta2-dev: e64cfa531121f2cf152901f5b2a20512a5ee2c63 < 3.3.0.beta1-dev: 35b0c2da37318f502bb7069ee8e3565eb3ba5dbf 3.1.999: e2cc21b33261ffeb32030e8ab5ea92775c911fbc < 3.2.0.beta1-dev: b36e2a5199db23e5f05374640b9690573a08b352 diff --git a/javascripts/discourse/components/brand-header-contents.gjs b/javascripts/discourse/components/brand-header-contents.gjs new file mode 100644 index 0000000..0eb9366 --- /dev/null +++ b/javascripts/discourse/components/brand-header-contents.gjs @@ -0,0 +1,79 @@ +import Component from "@glimmer/component"; +import { inject as service } from "@ember/service"; +import dIcon from "discourse-common/helpers/d-icon"; + +export default class BrandHeaderContents extends Component { + @service site; + + get shouldShow() { + return !this.site.mobileView || settings.show_bar_on_mobile; + } + + get brandLogo() { + const mobileView = this.site.mobileView; + const mobileLogoUrl = settings.mobile_logo_url || ""; + const showMobileLogo = mobileView && mobileLogoUrl.length > 0; + const logoUrl = settings.logo_url || ""; + const title = settings.brand_name; + + return { + url: showMobileLogo ? mobileLogoUrl : logoUrl, + title, + }; + } + + get hasIcons() { + return settings.icons.length > 0; + } + + get hasLinks() { + return settings.links.length > 0; + } + + +} diff --git a/javascripts/discourse/components/brand-header-contents.hbs b/javascripts/discourse/components/brand-header-contents.hbs deleted file mode 100644 index aa2fd76..0000000 --- a/javascripts/discourse/components/brand-header-contents.hbs +++ /dev/null @@ -1,44 +0,0 @@ -
- - {{#if this.brandLogo.url}} - - {{else}} - - {{/if}} - -
- -{{#if this.textLinks}} - -{{/if}} - -{{#if this.iconLinks}} -
- -
-{{/if}} \ No newline at end of file diff --git a/javascripts/discourse/components/brand-header-contents.js b/javascripts/discourse/components/brand-header-contents.js deleted file mode 100644 index 187c851..0000000 --- a/javascripts/discourse/components/brand-header-contents.js +++ /dev/null @@ -1,44 +0,0 @@ -import Component from "@glimmer/component"; -import { inject as service } from "@ember/service"; - -export default class BrandHeaderContents extends Component { - @service site; - - splitLinks(links) { - return links - ? links.split("|").map((l) => { - const values = l.split(","); - return { - rawLabel: values[0].trim(), - href: values[1].trim(), - target: (values[2] || "").trim(), - }; - }) - : []; - } - - get shouldShow() { - return !this.site.mobileView || settings.show_bar_on_mobile; - } - - get iconLinks() { - return this.splitLinks(settings.icons); - } - - get textLinks() { - return this.splitLinks(settings.links); - } - - get brandLogo() { - const mobileView = this.site.mobileView; - const mobileLogoUrl = settings.mobile_logo_url || ""; - const showMobileLogo = mobileView && mobileLogoUrl.length > 0; - const logoUrl = settings.logo_url || ""; - const title = settings.brand_name; - - return { - url: showMobileLogo ? mobileLogoUrl : logoUrl, - title, - }; - } -} diff --git a/locales/en.yml b/locales/en.yml new file mode 100644 index 0000000..eddcb11 --- /dev/null +++ b/locales/en.yml @@ -0,0 +1,29 @@ +en: + theme_metadata: + settings: + links: + description: Text links to be displayed in the header + schema: + properties: + text: + label: Text + description: The text to display for the link + url: + label: URL + description: The URL to link to + target: + label: Target + description: The target attribute for the link + icons: + description: Icon links to be displayed in the header + schema: + properties: + icon_name: + label: Icon + description: The name of the FontAwesome 5 icon to display for the link + url: + label: URL + description: The URL to link to + target: + label: Target + description: The target attribute for the link \ No newline at end of file diff --git a/migrations/settings/0001-migrate-links-setting.js b/migrations/settings/0001-migrate-links-setting.js new file mode 100644 index 0000000..11d17ac --- /dev/null +++ b/migrations/settings/0001-migrate-links-setting.js @@ -0,0 +1,31 @@ +export default function migrate(settings) { + const oldSetting = settings.get("links"); + + if (oldSetting) { + const newSetting = oldSetting.split("|").map((link) => { + let [name, url, target] = link.split(",").map((s) => s.trim()); + + if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) { + target = "_blank"; + } + + const newLink = { + name, + url, + target + } + + Object.keys(newLink).forEach((key) => { + if (newLink[key] === undefined) { + delete newLink[key]; + } + }); + + return newLink; + }) + + settings.set("links", newSetting); + } + + return settings; +} diff --git a/migrations/settings/0002-migrate-icons-setting.js b/migrations/settings/0002-migrate-icons-setting.js new file mode 100644 index 0000000..9693e9b --- /dev/null +++ b/migrations/settings/0002-migrate-icons-setting.js @@ -0,0 +1,31 @@ +export default function migrate(settings) { + const oldSetting = settings.get("icons"); + + if (oldSetting) { + const newSetting = oldSetting.split("|").map((link) => { + let [iconName, url, target] = link.split(",").map((s) => s.trim()); + + if (["_blank", "_self", "_parent", "_top"].indexOf(target) === -1) { + target = "_blank"; + } + + const newLink = { + icon_name: iconName, + url, + target + } + + Object.keys(newLink).forEach((key) => { + if (newLink[key] === undefined) { + delete newLink[key]; + } + }); + + return newLink; + }) + + settings.set("icons", newSetting); + } + + return settings; +} diff --git a/settings.yaml b/settings.yaml index 02b98b5..7ae978a 100644 --- a/settings.yaml +++ b/settings.yaml @@ -6,16 +6,63 @@ logo_url: default: "" mobile_logo_url: default: "" + links: - type: list - list_type: simple - default: "" - description: "Format: name,url,target (optional)" + type: objects + default: [] + schema: + name: link + properties: + text: + type: string + required: true + validations: + min: 1 + max: 100 + url: + type: string + required: true + validations: + min: 1 + max: 2048 + url: true + target: + type: enum + default: _blank + choices: + - _blank + - _self + - _parent + - _top + icons: - type: list - list_type: simple - default: "" - description: "Format: icon-name,url,target (optional)" + type: objects + default: [] + schema: + name: icon + properties: + icon_name: + type: string + required: true + validations: + min: 1 + max: 100 + url: + type: string + required: true + validations: + min: 1 + max: 2048 + url: true + target: + type: enum + default: _blank + choices: + - _blank + - _self + - _parent + - _top + custom_font_awesome_icons: type: list list_type: simple diff --git a/spec/system/viewing_brand_header_spec.rb b/spec/system/viewing_brand_header_spec.rb index 6d3ff6d..e7f1b57 100644 --- a/spec/system/viewing_brand_header_spec.rb +++ b/spec/system/viewing_brand_header_spec.rb @@ -27,13 +27,20 @@ theme.update_setting( :links, - "First Link,http://some.url.com/first|Second Link,http://some.url.com/second,_blank", + [ + { text: "First Link", url: "http://some.url.com/first", target: "_blank" }, + { text: "Second Link", url: "http://some.url.com/second", target: "_self" }, + ], ) theme.update_setting( :icons, - "wrench,http://some.url.com/some-wrench-link|pencil,http://some.url.com/some-pencil-link,_blank", + [ + { icon_name: "wrench", url: "http://some.url.com/some-wrench-link", target: "_self" }, + { icon_name: "pencil", url: "http://some.url.com/some-pencil-link", target: "_blank" }, + ], ) + theme.save! visit("/") @@ -44,10 +51,12 @@ 'img#brand-logo[title="some name"][src="http://some.url.com/logo.png"]', ) - expect(page).to have_link("First Link", href: "http://some.url.com/first") - expect(page).to have_link("Second Link", href: "http://some.url.com/second", target: "_blank") + expect(page).to have_link("First Link", href: "http://some.url.com/first", target: "_blank") + expect(page).to have_link("Second Link", href: "http://some.url.com/second", target: "_self") - expect(page).to have_selector('a[href="http://some.url.com/some-wrench-link"] .d-icon-wrench') + expect(page).to have_selector( + 'a[href="http://some.url.com/some-wrench-link"][target="_self"] .d-icon-wrench', + ) expect(page).to have_selector( 'a[href="http://some.url.com/some-pencil-link"][target="_blank"] .d-icon-pencil', diff --git a/test/unit/migrations/settings/0001-migrate-links-setting-test.js b/test/unit/migrations/settings/0001-migrate-links-setting-test.js new file mode 100644 index 0000000..e4b27fc --- /dev/null +++ b/test/unit/migrations/settings/0001-migrate-links-setting-test.js @@ -0,0 +1,42 @@ +import { module, test } from "qunit"; +import migrate from "../../../../migrations/settings/0001-migrate-links-setting"; + +module( + "Unit | Migrations | Settings | 0001-migrate-links-setting", + function () { + test("migrate", function (assert) { + const settings = new Map( + Object.entries({ + links: + "some, links |another, link, _blank|link2, /some/path, _invalid_target", + }) + ); + + const result = migrate(settings); + + const expectedResult = new Map( + Object.entries({ + links: [ + { + name: "some", + url: "links", + target: "_blank", + }, + { + name: "another", + url: "link", + target: "_blank", + }, + { + name: "link2", + url: "/some/path", + target: "_blank", + }, + ], + }) + ); + + assert.deepEqual(result, expectedResult); + }); + } +); diff --git a/test/unit/migrations/settings/0002-migrate-icons-setting-test.js b/test/unit/migrations/settings/0002-migrate-icons-setting-test.js new file mode 100644 index 0000000..1ec7cc9 --- /dev/null +++ b/test/unit/migrations/settings/0002-migrate-icons-setting-test.js @@ -0,0 +1,42 @@ +import { module, test } from "qunit"; +import migrate from "../../../../migrations/settings/0002-migrate-icons-setting"; + +module( + "Unit | Migrations | Settings | 0002-migrate-icons-setting", + function () { + test("migrate", function (assert) { + const settings = new Map( + Object.entries({ + icons: + "some-icon, http://some.url.com|another-icon, /some/path, _blank|icon-2, /some/path, _invalid_target", + }) + ); + + const result = migrate(settings); + + const expectedResult = new Map( + Object.entries({ + icons: [ + { + icon_name: "some-icon", + url: "http://some.url.com", + target: "_blank", + }, + { + icon_name: "another-icon", + url: "/some/path", + target: "_blank", + }, + { + icon_name: "icon-2", + url: "/some/path", + target: "_blank", + }, + ], + }) + ); + + assert.deepEqual(result, expectedResult); + }); + } +);