From f28f82f99ef4b3a7bceefcdd8eb7e935da5a50ac Mon Sep 17 00:00:00 2001
From: Jordan Vidrine <30537603+jordanvidrine@users.noreply.github.com>
Date: Wed, 16 Oct 2024 11:13:36 -0500
Subject: [PATCH] UX: Multiple fixes to theme card rendering (#29225)

* Improvements, refactors, remove dead code

---------

Co-authored-by: Martin Brennan <martin@discourse.org>
---
 .../components/admin-config-area-card.gjs     |   4 +-
 .../addon/components/themes-grid-card.gjs     | 203 +++++++-----------
 .../admin/addon/components/themes-grid.gjs    |  75 +++----
 .../javascripts/admin/addon/models/theme.js   |   1 +
 .../common/components/theme-card.scss         |  55 +++--
 config/locales/client.en.yml                  |   2 +-
 6 files changed, 161 insertions(+), 179 deletions(-)

diff --git a/app/assets/javascripts/admin/addon/components/admin-config-area-card.gjs b/app/assets/javascripts/admin/addon/components/admin-config-area-card.gjs
index 63d7687c938..ecb4396f648 100644
--- a/app/assets/javascripts/admin/addon/components/admin-config-area-card.gjs
+++ b/app/assets/javascripts/admin/addon/components/admin-config-area-card.gjs
@@ -24,7 +24,9 @@ export default class AdminConfigAreaCard extends Component {
             class="admin-config-area-card__title"
           >{{this.computedHeading}}</h3>
         {{else}}
-          <h3 class="admin-config-area-card__title">{{yield to="header"}}</h3>
+          {{#if (has-block "header")}}
+            <h3 class="admin-config-area-card__title">{{yield to="header"}}</h3>
+          {{/if}}
         {{/if}}
         {{#if (has-block "headerAction")}}
           <div class="admin-config-area-card__header-action">
diff --git a/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs b/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs
index 3dfa4ece261..33b50d80cf4 100644
--- a/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs
+++ b/app/assets/javascripts/admin/addon/components/themes-grid-card.gjs
@@ -1,6 +1,5 @@
 import Component from "@glimmer/component";
-import { Input } from "@ember/component";
-import { action, computed } from "@ember/object";
+import { action } from "@ember/object";
 import { service } from "@ember/service";
 import { htmlSafe } from "@ember/template";
 import DButton from "discourse/components/d-button";
@@ -21,54 +20,6 @@ export default class ThemeCard extends Component {
   @service siteSettings;
   @service toasts;
 
-  // NOTE: These 3 shouldn't need @computed, if we convert
-  // theme to a pure JS class with @tracked properties we
-  // won't need to do this.
-  @computed("args.theme.default")
-  get setDefaultButtonIcon() {
-    return this.args.theme.default ? "far-check-square" : "far-square";
-  }
-
-  @computed("args.theme.default")
-  get setDefaultButtonTitle() {
-    return this.args.theme.default
-      ? "admin.customize.theme.default_theme"
-      : "admin.customize.theme.set_default_theme";
-  }
-
-  @computed("args.theme.default")
-  get setDefaultButtonClasses() {
-    return this.args.theme.default
-      ? "btn-primary theme-card__button"
-      : "btn-default theme-card__button";
-  }
-
-  @computed(
-    "args.theme.default",
-    "args.theme.isBroken",
-    "args.theme.enabled",
-    "args.theme.isPendingUpdates"
-  )
-  get themeCardClasses() {
-    return this.args.theme.isBroken
-      ? "--broken"
-      : !this.args.theme.enabled
-      ? "--disabled"
-      : this.args.theme.isPendingUpdates
-      ? "--updates"
-      : this.args.theme.default
-      ? "--active"
-      : "";
-  }
-
-  get imageAlt() {
-    return this.args.theme.name;
-  }
-
-  get hasScreenshot() {
-    return this.args.theme.screenshot ? true : false;
-  }
-
   get themeRouteModels() {
     return ["themes", this.args.theme.id];
   }
@@ -92,91 +43,59 @@ export default class ThemeCard extends Component {
   // Will also need some cleanup when refactoring other theme code.
   @action
   async setDefault() {
-    this.args.theme.set("default", true);
-    this.args.theme.saveChanges("default").then(() => {
-      this.args.allThemes.forEach((theme) => {
-        if (theme.id !== this.args.theme.id) {
-          theme.set("default", !this.args.theme.get("default"));
-        }
-      });
-      this.toasts.success({
-        data: {
-          message: I18n.t("admin.customize.theme.set_default_success", {
-            theme: this.args.theme.name,
-          }),
-        },
-        duration: 2000,
-      });
-    });
-  }
+    let oldDefaultThemeId;
 
-  @action
-  async handleSubmit(event) {
-    this.args.theme.set("user_selectable", event.target.checked);
-    this.args.theme.saveChanges("user_selectable");
+    this.args.theme.set("default", true);
+    this.args.allThemes.forEach((theme) => {
+      if (theme.id !== this.args.theme.id) {
+        if (theme.get("default")) {
+          oldDefaultThemeId = theme.id;
+        }
+
+        theme.set("default", !this.args.theme.get("default"));
+      }
+    });
+
+    const changesSaved = await this.args.theme.saveChanges("default");
+    if (!changesSaved) {
+      this.args.allThemes
+        .find((theme) => theme.id === oldDefaultThemeId)
+        .set("default", true);
+      this.args.theme.set("default", false);
+      return;
+    }
+
+    this.toasts.success({
+      data: {
+        message: I18n.t("admin.customize.theme.set_default_success", {
+          theme: this.args.theme.name,
+        }),
+      },
+      duration: 2000,
+    });
   }
 
   <template>
     <AdminConfigAreaCard
-      class={{concatClass "theme-card" this.themeCardClasses}}
+      class={{concatClass "theme-card" (if @theme.default "-active")}}
+      @translatedHeading={{@theme.name}}
     >
-      <:header>
-        {{@theme.name}}
-        <span class="theme-card__icons">
-          {{#if @theme.isPendingUpdates}}
-            <DButton
-              @route="adminCustomizeThemes.show"
-              @routeModels={{this.themeRouteModels}}
-              @icon="sync"
-              @class="btn-flat theme-card__button"
-              @preventFocus={{true}}
-            />
-          {{else}}
-            {{#if @theme.isBroken}}
-              {{icon
-                "exclamation-circle"
-                class="broken-indicator"
-                title="admin.customize.theme.broken_theme_tooltip"
-              }}
-            {{/if}}
-            {{#unless @theme.enabled}}
-              {{icon
-                "ban"
-                class="light-grey-icon"
-                title="admin.customize.theme.disabled_component_tooltip"
-              }}
-            {{/unless}}
-          {{/if}}
-        </span>
-      </:header>
-      <:headerAction>
-        <Input
-          @type="checkbox"
-          @checked={{@theme.user_selectable}}
-          id="user-select-theme-{{@theme.id}}"
-          onclick={{this.handleSubmit}}
-        />
-        <label
-          class="theme-card__checkbox-label"
-          for="user-select-theme-{{@theme.id}}"
-        >
-          {{i18n "admin.config_areas.look_and_feel.themes.user_selectable"}}
-        </label>
-      </:headerAction>
       <:content>
         <div class="theme-card__image-wrapper">
-          {{#if this.hasScreenshot}}
+          {{#if @theme.screenshot}}
             <img
               class="theme-card__image"
               src={{htmlSafe @theme.screenshot}}
-              alt={{this.imageAlt}}
+              alt={{@theme.name}}
             />
           {{else}}
             <ThemesGridPlaceholder @theme={{@theme}} />
           {{/if}}
         </div>
         <div class="theme-card__content">
-          <p class="theme-card__description">{{@theme.description}}</p>
+          {{#if @theme.description}}
+            <p class="theme-card__description">{{@theme.description}}</p>
+          {{/if}}
           {{#if @theme.childThemes}}
             <span class="theme-card__components">{{i18n
                 "admin.customize.theme.components"
@@ -188,12 +107,52 @@ export default class ThemeCard extends Component {
           <DButton
             @action={{this.setDefault}}
             @preventFocus={{true}}
-            @icon={{this.setDefaultButtonIcon}}
-            @class={{this.setDefaultButtonClasses}}
-            @translatedLabel={{i18n this.setDefaultButtonTitle}}
+            @icon={{if @theme.default "far-check-square" "far-square"}}
+            @class={{concatClass
+              "theme-card__button"
+              (if @theme.default "btn-primary" "btn-default")
+            }}
+            @translatedLabel={{i18n
+              (if
+                @theme.default
+                "admin.customize.theme.default_theme"
+                "admin.customize.theme.set_default_theme"
+              )
+            }}
             @disabled={{@theme.default}}
           />
-          <div class="theme-card-footer__actions">
+          {{#if @theme.isPendingUpdates}}
+            <DButton
+              @route="adminCustomizeThemes.show"
+              @routeModels={{this.themeRouteModels}}
+              @icon="sync"
+              @class="btn btn-flat theme-card__button"
+              @title="admin.customize.theme.updates_available_tooltip"
+              @preventFocus={{true}}
+            />
+          {{else}}
+            {{#if @theme.isBroken}}
+              <DButton
+                @route="adminCustomizeThemes.show"
+                @routeModels={{this.themeRouteModels}}
+                @icon="exclamation-circle"
+                @class="btn btn-flat theme-card__button broken-indicator"
+                @title="admin.customize.theme.broken_theme_tooltip"
+                @preventFocus={{true}}
+              />
+            {{/if}}
+            {{#unless @theme.enabled}}
+              <DButton
+                @route="adminCustomizeThemes.show"
+                @routeModels={{this.themeRouteModels}}
+                @icon="ban"
+                @class="btn btn-flat theme-card__button broken-indicator light-grey-icon"
+                @title="admin.customize.theme.disabled_component_tooltip"
+                @preventFocus={{true}}
+              />
+            {{/unless}}
+          {{/if}}
+          <div class="theme-card__footer-actions">
             <a
               href={{this.themePreviewUrl}}
               title={{i18n "admin.customize.explain_preview"}}
diff --git a/app/assets/javascripts/admin/addon/components/themes-grid.gjs b/app/assets/javascripts/admin/addon/components/themes-grid.gjs
index 3ec57d870ce..c810318ac72 100644
--- a/app/assets/javascripts/admin/addon/components/themes-grid.gjs
+++ b/app/assets/javascripts/admin/addon/components/themes-grid.gjs
@@ -31,12 +31,12 @@ export default class ThemesGrid extends Component {
     },
   ];
 
+  // Always show the default theme first in the list
   get sortedThemes() {
-    // Always show currently set default theme first
     return this.args.themes.sort((a, b) => {
-      if (a.default) {
+      if (a.get("default")) {
         return -1;
-      } else if (b.default) {
+      } else if (b.get("default")) {
         return 1;
       }
     });
@@ -78,39 +78,42 @@ export default class ThemesGrid extends Component {
 
   <template>
     <div class="themes-cards-container">
-      {{#each this.sortedThemes as |theme|}}
-        <ThemesGridCard @theme={{theme}} @allThemes={{@themes}} />
-      {{/each}}
-
-      <AdminConfigAreaCard class="theme-card">
-        <:content>
-          <h2 class="theme-card__title">{{i18n
-              "admin.config_areas.look_and_feel.themes.new_theme"
-            }}</h2>
-          <p class="theme-card__description">{{i18n
-              "admin.customize.theme.themes_intro_new"
-            }}</p>
-          <div class="external-resources">
-            {{#each this.externalResources as |resource|}}
-              <a
-                href={{resource.link}}
-                class="external-link"
-                rel="noopener noreferrer"
-                target="_blank"
-              >
-                {{i18n resource.key}}
-                {{icon "external-link-alt"}}
-              </a>
-            {{/each}}
-          </div>
-          <DButton
-            @action={{this.installModal}}
-            @icon="upload"
-            @label="admin.customize.install"
-            class="btn-primary theme-card__button"
-          />
-        </:content>
-      </AdminConfigAreaCard>
+      <div class="themes-cards-container__main">
+        {{#each this.sortedThemes as |theme|}}
+          <ThemesGridCard @theme={{theme}} @allThemes={{@themes}} />
+        {{/each}}
+      </div>
+      <div class="themes-cards-container__helper">
+        <AdminConfigAreaCard
+          class="theme-card"
+          @heading="admin.config_areas.look_and_feel.themes.new_theme"
+        >
+          <:content>
+            <p class="theme-card__description">{{i18n
+                "admin.customize.theme.themes_intro_new"
+              }}</p>
+            <div class="external-resources">
+              {{#each this.externalResources as |resource|}}
+                <a
+                  href={{resource.link}}
+                  class="external-link"
+                  rel="noopener noreferrer"
+                  target="_blank"
+                >
+                  {{i18n resource.key}}
+                  {{icon "external-link-alt"}}
+                </a>
+              {{/each}}
+            </div>
+            <DButton
+              @action={{this.installModal}}
+              @icon="upload"
+              @label="admin.customize.install"
+              class="btn-primary theme-card__install-button"
+            />
+          </:content>
+        </AdminConfigAreaCard>
+      </div>
     </div>
   </template>
 }
diff --git a/app/assets/javascripts/admin/addon/models/theme.js b/app/assets/javascripts/admin/addon/models/theme.js
index ced9cce4fb8..b7ca8babb06 100644
--- a/app/assets/javascripts/admin/addon/models/theme.js
+++ b/app/assets/javascripts/admin/addon/models/theme.js
@@ -317,6 +317,7 @@ class Theme extends RestModel {
   saveChanges() {
     const hash = this.getProperties.apply(this, arguments);
     return this.save(hash)
+      .then(() => true)
       .finally(() => this.set("changed", false))
       .catch(popupAjaxError);
   }
diff --git a/app/assets/stylesheets/common/components/theme-card.scss b/app/assets/stylesheets/common/components/theme-card.scss
index 8a561dfd406..3cc90e9eeaf 100644
--- a/app/assets/stylesheets/common/components/theme-card.scss
+++ b/app/assets/stylesheets/common/components/theme-card.scss
@@ -5,8 +5,19 @@
 
 .themes-cards-container {
   display: grid;
-  grid-template-columns: repeat(auto-fill, minmax(340px, 1fr));
+  grid-template-columns: calc(66.66% - 0.5em) calc(33.33% - 0.5em);
   gap: 1em;
+
+  @media screen and (max-width: 1300px) {
+    display: flex;
+    flex-direction: column-reverse;
+  }
+
+  &__main {
+    display: grid;
+    grid-template-columns: repeat(auto-fill, minmax(calc(325px - 1em), 1fr));
+    gap: 1em;
+  }
 }
 
 .theme-card {
@@ -19,22 +30,18 @@
     flex-grow: 1;
   }
 
-  &.--active {
+  &.-active {
     @include theme-card-border(tertiary);
   }
 
-  &.--disabled {
+  &.-disabled {
     @include theme-card-border(primary);
   }
 
-  &.--broken {
+  &.-broken {
     @include theme-card-border(danger);
   }
 
-  &.--updates {
-    @include theme-card-border(success);
-  }
-
   .broken-indicator {
     color: var(--danger);
   }
@@ -43,10 +50,6 @@
     display: flex;
   }
 
-  &__icons .btn-flat {
-    padding: 0;
-  }
-
   &__image-wrapper {
     width: 100%;
     height: 160px;
@@ -76,21 +79,34 @@
     font-weight: 400;
   }
 
-  &__components {
-    display: block;
-    font-size: var(--font-down-1);
-    margin-bottom: 10px;
-    color: var(--primary-high);
+  &__content {
+    padding: 10px 0;
   }
 
-  &__button {
+  &__description {
+    margin: 0 0 10px 0;
+  }
+
+  &__components {
+    display: -webkit-box;
+    font-size: var(--font-down-1);
+    color: var(--primary-high);
+    -webkit-box-orient: vertical;
+    overflow: hidden;
+    -webkit-line-clamp: 3;
+  }
+
+  &__install-button {
     margin-top: auto;
   }
 
   &__footer {
     margin-top: auto;
     display: flex;
-    justify-content: space-between;
+  }
+
+  &__footer-actions {
+    margin-left: auto;
   }
 
   .admin-config-area-card__header-action {
@@ -110,6 +126,7 @@
   justify-content: space-between;
   flex-direction: column;
   font-size: var(--font-down-1);
+  margin-bottom: 10px;
 
   .external-link {
     margin-bottom: 0.25em;
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index 976e63cdae7..5e0a7388631 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -5668,7 +5668,7 @@ en:
             move_up: "Move up"
             move_down: "Move down"
         look_and_feel:
-          title: "Look & Feel"
+          title: "Look & feel"
           description: "Themes, components, and color schemes can be used to customise and brand your Discourse site, giving it a distinctive style."
           themes:
             title: "Themes"