FEATURE: direct link to components for admin sidebar (#26644)

To add a components link to the sidebar refactoring was required to create unique URLs for themes and components. Before the query param was used. After changes, we have two URLs `/admin/customize/themes` and `/admin/customize/components`.
discourse · Apr 17, 2024 · df373d9 · df373d9
1 parent 56c4804
commit df373d9
Show file tree

Hide file tree

Showing 12 changed files with 79 additions and 117 deletions.
diff --git a/app/assets/javascripts/admin/addon/components/themes-list.js b/app/assets/javascripts/admin/addon/components/themes-list.js
@@ -77,6 +77,13 @@ export default class ThemesList extends Component {
   @equal("filter", ACTIVE_FILTER) activeFilter;
   @equal("filter", INACTIVE_FILTER) inactiveFilter;
 
+  willRender() {
+    super.willRender(...arguments);
+    if (!this.showSearchAndFilter) {
+      this.set("searchTerm", null);
+    }
+  }
+
   @discourseComputed("themes", "components", "currentTab")
   themesList(themes, components) {
     if (this.themesTabActive) {
@@ -208,11 +215,8 @@ export default class ThemesList extends Component {
   changeView(newTab) {
     if (newTab !== this.currentTab) {
       this.set("selectInactiveMode", false);
-      this.set("currentTab", newTab);
       this.set("filter", ALL_FILTER);
-      if (!this.showSearchAndFilter) {
-        this.set("searchTerm", null);
-      }
+      this.router.transitionTo("adminCustomizeThemes", newTab);
     }
   }
 

diff --git a/app/assets/javascripts/admin/addon/routes/admin-customize-theme-components.js b/app/assets/javascripts/admin/addon/routes/admin-customize-theme-components.js
diff --git a/app/assets/javascripts/admin/addon/routes/admin-customize-themes.js b/app/assets/javascripts/admin/addon/routes/admin-customize-themes.js
@@ -13,23 +13,20 @@ export default class AdminCustomizeThemesRoute extends Route {
   queryParams = {
     repoUrl: null,
     repoName: null,
-    tab: null,
   };
 
-  model() {
+  model(params) {
+    this.currentTab = params.type;
     return this.store.findAll("theme");
   }
 
   setupController(controller, model) {
     super.setupController(controller, model);
 
-    if (controller.tab) {
+    if (this.currentTab) {
       controller.setProperties({
         editingTheme: false,
-        currentTab: controller.tab,
-
-        // this is to get rid of the queryString since we don't want it hanging around
-        tab: undefined,
+        currentTab: this.currentTab,
       });
     }
 

diff --git a/app/assets/javascripts/admin/addon/routes/admin-route-map.js b/app/assets/javascripts/admin/addon/routes/admin-route-map.js
@@ -55,7 +55,7 @@ export default function () {
 
         this.route(
           "adminCustomizeThemes",
-          { path: "themes", resetNamespace: true },
+          { path: "/:type", resetNamespace: true },
           function () {
             this.route("show", { path: "/:theme_id" }, function () {
               this.route("schema", { path: "schema/:setting_name" });
@@ -64,11 +64,6 @@ export default function () {
           }
         );
 
-        this.route("adminCustomizeThemeComponents", {
-          path: "theme-components",
-          resetNamespace: true,
-        });
-
         this.route(
           "adminSiteText",
           { path: "/site_texts", resetNamespace: true },

diff --git a/app/assets/javascripts/admin/addon/templates/admin.hbs b/app/assets/javascripts/admin/addon/templates/admin.hbs
@@ -25,7 +25,11 @@
             <NavItem @route="adminEmail" @label="admin.email.title" />
           {{/if}}
           <NavItem @route="adminLogs" @label="admin.logs.title" />
-          <NavItem @route="adminCustomize" @label="admin.customize.title" />
+          <NavItem
+            @route="adminCustomizeThemes"
+            @routeParam="themes"
+            @label="admin.customize.title"
+          />
           {{#if this.currentUser.admin}}
             <NavItem @route="adminApi" @label="admin.api.title" />
             {{#if this.siteSettings.enable_backups}}

diff --git a/app/assets/javascripts/admin/addon/templates/customize.hbs b/app/assets/javascripts/admin/addon/templates/customize.hbs
@@ -3,6 +3,7 @@
     {{#if this.currentUser.admin}}
       <NavItem
         @route="adminCustomizeThemes"
+        @routeParam="themes"
         @label="admin.customize.theme.title"
         class="admin-customize-themes"
       />

diff --git a/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js b/app/assets/javascripts/discourse/app/lib/sidebar/admin-nav-map.js
@@ -150,10 +150,18 @@ export const ADMIN_NAV_MAP = [
       {
         name: "admin_themes",
         route: "adminCustomizeThemes",
-        query: { tab: "themes" },
+        routeModels: ["themes"],
+        model: "themes",
         label: "admin.appearance.sidebar_link.themes",
         icon: "paint-brush",
       },
+      {
+        name: "admin_components",
+        route: "adminCustomizeThemes",
+        routeModels: ["components"],
+        label: "admin.appearance.sidebar_link.components",
+        icon: "puzzle-piece",
+      },
       {
         name: "admin_customize_site_texts",
         route: "adminSiteText",

diff --git a/app/assets/javascripts/discourse/tests/integration/components/themes-list-test.js b/app/assets/javascripts/discourse/tests/integration/components/themes-list-test.js
@@ -1,4 +1,4 @@
-import { click, fillIn, render } from "@ember/test-helpers";
+import { fillIn, render } from "@ember/test-helpers";
 import { hbs } from "ember-cli-htmlbars";
 import { module, test } from "qunit";
 import { setupRenderingTest } from "discourse/tests/helpers/component-test";
@@ -367,86 +367,4 @@ module("Integration | Component | themes-list", function (hooks) {
     );
     assert.deepEqual(componentNames(), ["Component unused and disabled 1"]);
   });
-
-  test("switching between themes and components tabs keeps the search visible only if both tabs have at least 10 items", async function (assert) {
-    const themes = createThemes(10, (n) => {
-      return { name: `Theme ${n}${n}` };
-    });
-    const components = createThemes(5, (n) => {
-      return {
-        name: `Component ${n}${n}`,
-        component: true,
-        parent_themes: [1],
-        parentThemes: [1],
-      };
-    });
-    this.setProperties({
-      themes,
-      components,
-      currentTab: THEMES,
-    });
-
-    await render(
-      hbs`<ThemesList @themes={{this.themes}} @components={{this.components}} @currentTab={{this.currentTab}} />`
-    );
-
-    await fillIn(".themes-list-search__input", "11");
-    assert.strictEqual(
-      query(".themes-list-container__item .info").textContent.trim(),
-      "Theme 11",
-      "only 1 theme is shown"
-    );
-    await click(".themes-list-header .components-tab");
-    assert.ok(
-      !exists(".themes-list-search__input"),
-      "search input/term do not persist when we switch to the other" +
-        " tab because it has fewer than 10 items"
-    );
-    assert.deepEqual(
-      [...queryAll(".themes-list-container__item .info .name")].map((node) =>
-        node.textContent.trim()
-      ),
-      [
-        "Component 11",
-        "Component 22",
-        "Component 33",
-        "Component 44",
-        "Component 55",
-      ],
-      "all components are shown"
-    );
-
-    this.set(
-      "components",
-      this.components.concat(
-        createThemes(5, (n) => {
-          n += 5;
-          return {
-            name: `Component ${n}${n}`,
-            component: true,
-            parent_themes: [1],
-            parentThemes: [1],
-          };
-        })
-      )
-    );
-    assert.ok(
-      exists(".themes-list-search__input"),
-      "search is now shown for the components tab"
-    );
-
-    await fillIn(".themes-list-search__input", "66");
-    assert.strictEqual(
-      query(".themes-list-container__item .info").textContent.trim(),
-      "Component 66",
-      "only 1 component is shown"
-    );
-
-    await click(".themes-list-header .themes-tab");
-    assert.strictEqual(
-      query(".themes-list-container__item .info").textContent.trim(),
-      "Theme 66",
-      "search term persisted between tabs because both have more than 10 items"
-    );
-  });
 });
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
@@ -5337,6 +5337,7 @@ en:
           emoji: "Emoji"
           navigation: "Navigation"
           themes: "Themes"
+          components: "Components"
           site_texts: "Site Texts"
 
       email_settings:

diff --git a/config/routes.rb b/config/routes.rb
@@ -221,6 +221,7 @@ def patch(*)
 
       get "customize" => "color_schemes#index", :constraints => AdminConstraint.new
       get "customize/themes" => "themes#index", :constraints => AdminConstraint.new
+      get "customize/components" => "themes#index", :constraints => AdminConstraint.new
       get "customize/theme-components" => "themes#index", :constraints => AdminConstraint.new
       get "customize/colors" => "color_schemes#index", :constraints => AdminConstraint.new
       get "customize/colors/:id" => "color_schemes#index", :constraints => AdminConstraint.new
@@ -257,6 +258,7 @@ def patch(*)
 
         get "themes/:id/:target/:field_name/edit" => "themes#index"
         get "themes/:id" => "themes#index"
+        get "components/:id" => "themes#index"
         get "themes/:id/export" => "themes#export"
         get "themes/:id/schema/:setting_name" => "themes#schema"
 

diff --git a/spec/system/admin_customize_themes_spec.rb b/spec/system/admin_customize_themes_spec.rb
@@ -48,9 +48,35 @@
     end
 
     it "selects the component tab when visiting the theme-components route" do
-      visit("/admin/customize/theme-components")
+      visit("/admin/customize/components")
       expect(find(".themes-list-header")).to have_css(".components-tab.active")
     end
+
+    it "switching between themes and components tabs keeps the search visible only if both tabs have at least 10 items" do
+      6.times { Fabricate(:theme) }
+      (1..5).each { |number| Fabricate(:theme, component: true, name: "Cool component #{number}") }
+
+      visit("/admin/customize/themes")
+      expect(admin_customize_themes_page).to have_themes(count: 11)
+
+      admin_customize_themes_page.search("5")
+      expect(admin_customize_themes_page).to have_themes(count: 1)
+
+      admin_customize_themes_page.switch_to_components
+      expect(admin_customize_themes_page).to have_no_search
+      expect(admin_customize_themes_page).to have_themes(count: 5)
+
+      (6..11).each { |number| Fabricate(:theme, component: true, name: "Cool component #{number}") }
+
+      visit("/admin/customize/components")
+      expect(admin_customize_themes_page).to have_themes(count: 11)
+
+      admin_customize_themes_page.search("5")
+      expect(admin_customize_themes_page).to have_themes(count: 1)
+
+      admin_customize_themes_page.switch_to_themes
+      expect(admin_customize_themes_page).to have_themes(count: 1)
+    end
   end
 
   describe "when visiting the page to customize a single theme" do

diff --git a/spec/system/page_objects/pages/admin_customize_themes.rb b/spec/system/page_objects/pages/admin_customize_themes.rb
@@ -45,6 +45,10 @@ def has_inactive_themes_selected?(count:)
         has_css?(".inactive-theme input:checked", count: count)
       end
 
+      def has_themes?(count:)
+        has_css?(".themes-list-container__item", count: count)
+      end
+
       def toggle_all_inactive
         find(".toggle-all-inactive").click
       end
@@ -67,6 +71,22 @@ def click_theme_settings_editor_button
         PageObjects::Components::AdminThemeSettingsEditor.new
       end
 
+      def switch_to_components
+        find(".components-tab").click
+      end
+
+      def switch_to_themes
+        find(".themes-tab").click
+      end
+
+      def search(term)
+        find(".themes-list-search__input").fill_in with: term
+      end
+
+      def has_no_search?
+        has_no_css?(".themes-list-search__input")
+      end
+
       private
 
       def setting_selector(setting_name, overridden: false)