From 3402e1c8182fa2d5fed33c8bf4eaea131af4124e Mon Sep 17 00:00:00 2001
From: Kris <kris.aubuchon@discourse.org>
Date: Thu, 12 Dec 2024 17:01:02 -0500
Subject: [PATCH] DEV: Refactor for readability and add tests for logo display
 logic (#65)

Co-authored-by: Jarek Radosz <jradosz@gmail.com>
---
 .../components/brand-header-contents.gjs      | 64 ++++++++-----------
 spec/system/viewing_brand_header_spec.rb      | 27 ++++++++
 2 files changed, 53 insertions(+), 38 deletions(-)

diff --git a/javascripts/discourse/components/brand-header-contents.gjs b/javascripts/discourse/components/brand-header-contents.gjs
index 86591bb..1d466b0 100644
--- a/javascripts/discourse/components/brand-header-contents.gjs
+++ b/javascripts/discourse/components/brand-header-contents.gjs
@@ -7,66 +7,54 @@ export default class BrandHeaderContents extends Component {
   @service site;
 
   get shouldShow() {
-    return !this.site.mobileView || settings.show_bar_on_mobile;
+    return this.site.desktopView || 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 logoDarkUrl = settings.logo_dark_url || "";
-    const title = settings.brand_name;
-
-    return {
-      mobileUrl: showMobileLogo ? mobileLogoUrl : null,
-      lightImg: {
-        url: logoUrl,
-      },
-      darkImg: {
-        url: logoDarkUrl,
-      },
-      title,
-    };
+  get mobileLogoUrl() {
+    return this.site.mobileView ? settings.mobile_logo_url : null;
   }
 
-  get hasIcons() {
-    return settings.icons && settings.icons.length > 0;
+  get lightLogo() {
+    return { url: settings.logo_url || "" };
   }
 
-  get hasLinks() {
-    return settings.links && settings.links.length > 0;
+  get darkLogo() {
+    return { url: settings.logo_dark_url || "" };
   }
 
   <template>
     <div class="title">
       <a href={{settings.website_url}}>
-        {{#if this.brandLogo.mobileUrl}}
+        {{#if this.mobileLogoUrl}}
           <img
             id="brand-logo"
             class="logo-big"
-            src={{this.brandLogo.mobileUrl}}
-            title={{this.brandLogo.title}}
+            src={{this.mobileLogoUrl}}
+            title={{settings.brand_name}}
           />
-        {{else}}
+        {{else if this.lightLogo.url}}
           <LightDarkImg
             id="brand-logo"
             class="logo-big"
-            @lightImg={{this.brandLogo.lightImg}}
-            @darkImg={{this.brandLogo.darkImg}}
-            title={{this.brandLogo.title}}
+            @lightImg={{this.lightLogo}}
+            @darkImg={{this.darkLogo}}
+            title={{settings.brand_name}}
           />
+        {{else}}
+          <h2 id="brand-text-logo" class="text-logo">
+            {{settings.brand_name}}
+          </h2>
         {{/if}}
       </a>
     </div>
 
-    {{#if this.hasLinks}}
+    {{#if settings.links}}
       <nav class="links">
         <ul class="nav {{if this.shouldShow 'nav-pills'}}">
-          {{#each settings.links as |tl|}}
+          {{#each settings.links as |link|}}
             <li>
-              <a href={{tl.url}} target={{tl.target}}>
-                {{tl.text}}
+              <a href={{link.url}} target={{link.target}}>
+                {{link.text}}
               </a>
             </li>
           {{/each}}
@@ -74,13 +62,13 @@ export default class BrandHeaderContents extends Component {
       </nav>
     {{/if}}
 
-    {{#if this.hasIcons}}
+    {{#if settings.icons}}
       <div class="panel">
         <ul class="icons">
-          {{#each settings.icons as |il|}}
+          {{#each settings.icons as |iconLink|}}
             <li>
-              <a href={{il.url}} target={{il.target}}>
-                {{dIcon il.icon_name}}
+              <a href={{iconLink.url}} target={{iconLink.target}}>
+                {{dIcon iconLink.icon_name}}
               </a>
             </li>
           {{/each}}
diff --git a/spec/system/viewing_brand_header_spec.rb b/spec/system/viewing_brand_header_spec.rb
index 056c4f2..be75fd4 100644
--- a/spec/system/viewing_brand_header_spec.rb
+++ b/spec/system/viewing_brand_header_spec.rb
@@ -20,6 +20,15 @@
     expect(page).not_to have_css(".b-header")
   end
 
+  it "renders as a dropdown on mobile", mobile: true do
+    theme.update_setting(:show_bar_on_mobile, false)
+    theme.save!
+
+    visit("/")
+
+    expect(page).to have_css("#toggle-hamburger-brand-menu")
+  end
+
   it "should display the brand header with the correct title and links" do
     theme.update_setting(:website_url, "http://some.url.com")
     theme.update_setting(:brand_name, "some name")
@@ -63,4 +72,22 @@
       'a[href="http://some.url.com/some-pencil-link"][target="_blank"] .d-icon-pencil',
     )
   end
+
+  it "shows the brand name when no logo is uploaded" do
+    theme.update_setting(:brand_name, "some name")
+    theme.save!
+
+    visit("/")
+
+    expect(page).to have_css("#brand-text-logo", text: "some name")
+  end
+
+  it "does not show the brand name when a logo is uploaded" do
+    theme.update_setting(:logo_url, "http://example.com/logo.png")
+    theme.save!
+
+    visit("/")
+
+    expect(page).to have_css('img#brand-logo[src="http://example.com/logo.png"]', visible: :all)
+  end
 end