From 53eb49de72718196e534d2f92edd4fe418682d51 Mon Sep 17 00:00:00 2001
From: Alan Guo Xiang Tan <gxtan1990@gmail.com>
Date: Tue, 21 Feb 2023 10:29:44 +0800
Subject: [PATCH] DEV: Use data attributes as identifier for sidebar category
 section links (#20384)

Why this change?

Prior to this change, we placed the identifier for the category using
CSS classes like `sidebar-section-link-<category slug>`. However, we found that it wasn't obvious
that an identifier for the category exists since it is first buried in
the CSS classes and second there isn't a way to describe what the
identifier is. Using data attributes, it makes it more obvious that an
identifier exists and what the identifier represents.
---
 .../sidebar/anonymous/categories-section.hbs  |  2 +-
 .../app/components/sidebar/section-link.hbs   |  2 +
 .../app/components/sidebar/section-link.js    | 10 +--
 .../sidebar/user/categories-section.hbs       |  2 +-
 .../acceptance/sidebar-plugin-api-test.js     | 17 ++--
 .../sidebar-user-categories-section-test.js   | 86 ++++++++++++-------
 .../user-preferences-sidebar-test.js          | 28 ++++--
 7 files changed, 95 insertions(+), 52 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/categories-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/categories-section.hbs
index 82853f2bd7b..11e8866c0b1 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/anonymous/categories-section.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/anonymous/categories-section.hbs
@@ -6,7 +6,6 @@
 
   {{#each this.sectionLinks as |sectionLink|}}
     <Sidebar::SectionLink
-      @linkName={{sectionLink.name}}
       @route={{sectionLink.route}}
       @title={{sectionLink.title}}
       @content={{sectionLink.text}}
@@ -16,6 +15,7 @@
       @prefixValue={{sectionLink.prefixValue}}
       @prefixColor={{sectionLink.prefixColor}}
       @prefixElementColors={{sectionLink.prefixElementColors}}
+      data-category-id={{sectionLink.category.id}}
     />
   {{/each}}
 
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs b/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs
index ff18258e889..4d3ba543b5b 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs
@@ -10,6 +10,7 @@
         target="_blank"
         class={{this.classNames}}
         title={{@title}}
+        ...attributes
       >
         <Sidebar::SectionLinkPrefix
           @prefixType={{@prefixType}}
@@ -32,6 +33,7 @@
         @models={{this.models}}
         @current-when={{@currentWhen}}
         title={{@title}}
+        ...attributes
       >
 
         <Sidebar::SectionLinkPrefix
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
index 620da26bd64..41494f407ae 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
+++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
@@ -22,11 +22,11 @@ export default class SectionLink extends Component {
   }
 
   get classNames() {
-    let classNames = [
-      "sidebar-section-link",
-      `sidebar-section-link-${this.args.linkName}`,
-      "sidebar-row",
-    ];
+    let classNames = ["sidebar-section-link", "sidebar-row"];
+
+    if (this.args.linkName) {
+      classNames.push(`sidebar-section-link-${this.args.linkName}`);
+    }
 
     if (this.args.class) {
       classNames.push(this.args.class);
diff --git a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs
index 1e1ebeff991..a48a31c7787 100644
--- a/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/user/categories-section.hbs
@@ -15,7 +15,6 @@
     {{#if (gt this.sectionLinks.length 0)}}
       {{#each this.sectionLinks as |sectionLink|}}
         <Sidebar::SectionLink
-          @linkName={{sectionLink.name}}
           @route={{sectionLink.route}}
           @query={{sectionLink.query}}
           @title={{sectionLink.title}}
@@ -31,6 +30,7 @@
           @suffixCSSClass={{sectionLink.suffixCSSClass}}
           @suffixValue={{sectionLink.suffixValue}}
           @suffixType={{sectionLink.suffixType}}
+          data-category-id={{sectionLink.category.id}}
         />
       {{/each}}
     {{else}}
diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-plugin-api-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-plugin-api-test.js
index 70f063461e5..8b601f45a81 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-plugin-api-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-plugin-api-test.js
@@ -684,20 +684,21 @@ acceptance("Sidebar - Plugin API", function (needs) {
 
         assert.ok(
           exists(
-            `.sidebar-section-link-${category1.name} .sidebar-section-link-suffix.unread`
+            `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-suffix.unread`
           ),
           "the right suffix is displayed when custom countable is active"
         );
 
         assert.strictEqual(
-          query(`.sidebar-section-link-${category1.name}`).pathname,
+          query(`.sidebar-section-link[data-category-id="${category1.id}"]`)
+            .pathname,
           `/c/${category1.name}/${category1.id}`,
           "does not use route configured for custom countable when user has elected not to show any counts in sidebar"
         );
 
         assert.notOk(
           exists(
-            `.sidebar-section-link-${category2.name} .sidebar-section-link-suffix.unread`
+            `.sidebar-section-link[data-category-id="${category2.id}"] .sidebar-section-link-suffix.unread`
           ),
           "does not display suffix when custom countable is not registered"
         );
@@ -708,7 +709,7 @@ acceptance("Sidebar - Plugin API", function (needs) {
 
         assert.strictEqual(
           query(
-            `.sidebar-section-link-${category1.name} .sidebar-section-link-content-badge`
+            `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-content-badge`
           ).innerText.trim(),
           I18n.t("sidebar.unread_count", { count: 1 }),
           "displays the right badge text in section link when unread is present and custom countable is not prioritised over unread"
@@ -722,20 +723,22 @@ acceptance("Sidebar - Plugin API", function (needs) {
 
         assert.strictEqual(
           query(
-            `.sidebar-section-link-${category1.name} .sidebar-section-link-content-badge`
+            `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-content-badge`
           ).innerText.trim(),
           `some custom ${category1.topic_count}`,
           "displays the right badge text in section link when unread is present but custom countable is prioritised over unread"
         );
 
         assert.strictEqual(
-          query(`.sidebar-section-link-${category1.name}`).pathname,
+          query(`.sidebar-section-link[data-category-id="${category1.id}"]`)
+            .pathname,
           `/c/${category1.name}/${category1.id}/l/latest`,
           "has the right pathname for section link"
         );
 
         assert.strictEqual(
-          query(`.sidebar-section-link-${category1.name}`).search,
+          query(`.sidebar-section-link[data-category-id="${category1.id}"]`)
+            .search,
           "?status=open",
           "has the right query params for section link"
         );
diff --git a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js
index dd182b99b9c..704cf303a3f 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/sidebar-user-categories-section-test.js
@@ -50,7 +50,7 @@ acceptance(
       );
 
       assert.ok(
-        exists(`.sidebar-section-link-${category1.slug}`),
+        exists(`.sidebar-section-link[data-category-id="${category1.id}"]`),
         `only the ${category1.slug} section link is shown`
       );
     });
@@ -187,7 +187,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     await visit("/");
 
     assert.ok(
-      exists(`.sidebar-section-link-${uncategorizedCategory.slug}`),
+      exists(
+        `.sidebar-section-link[data-category-id="${uncategorizedCategory.id}"]`
+      ),
       `displays the section link for ${uncategorizedCategory.slug} category`
     );
   });
@@ -397,18 +399,20 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #${category1.color} 50%, #${category1.color} 50%)"]`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #${category1.color} 50%, #${category1.color} 50%)"]`
       ),
       "category1 section link is rendered with solid prefix icon color"
     );
 
     assert.strictEqual(
-      query(`.sidebar-section-link-${category1.slug}`).textContent.trim(),
+      query(
+        `.sidebar-section-link[data-category-id="${category1.id}"]`
+      ).textContent.trim(),
       category1.name,
       "displays category1's name for the link text"
     );
 
-    await click(`.sidebar-section-link-${category1.slug}`);
+    await click(`.sidebar-section-link[data-category-id="${category1.id}"]`);
 
     assert.strictEqual(
       currentURL(),
@@ -423,11 +427,13 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active"
     );
 
-    await click(`.sidebar-section-link-${category2.slug}`);
+    await click(`.sidebar-section-link[data-category-id="${category2.id}"]`);
 
     assert.strictEqual(
       currentURL(),
@@ -442,20 +448,22 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category2.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category2.id}"].active`
+      ),
       "the category2 section link is marked as active"
     );
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category3.slug} .sidebar-section-link-prefix .prefix-badge.d-icon-lock`
+        `.sidebar-section-link[data-category-id="${category3.id}"] .sidebar-section-link-prefix .prefix-badge.d-icon-lock`
       ),
       "category3 section link is rendered with lock prefix badge icon as it is read restricted"
     );
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category4.slug} .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #${category4.parentCategory.color} 50%, #${category4.color} 50%)"]`
+        `.sidebar-section-link[data-category-id="${category4.id}"] .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #${category4.parentCategory.color} 50%, #${category4.color} 50%)"]`
       ),
       "sub category section link is rendered with double prefix color"
     );
@@ -469,7 +477,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     await visit("/");
 
-    await click(`.sidebar-section-link-${category1.slug}`);
+    await click(`.sidebar-section-link[data-category-id="${category1.id}"]`);
 
     assert.strictEqual(
       currentURL(),
@@ -484,7 +492,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active"
     );
   });
@@ -507,7 +517,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     await visit("/");
 
-    await click(`.sidebar-section-link-${category1.slug}`);
+    await click(`.sidebar-section-link[data-category-id="${category1.id}"]`);
 
     assert.strictEqual(
       currentURL(),
@@ -522,7 +532,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active"
     );
   });
@@ -553,7 +565,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     await visit("/");
 
-    await click(`.sidebar-section-link-${category1.slug}`);
+    await click(`.sidebar-section-link[data-category-id="${category1.id}"]`);
 
     assert.strictEqual(
       currentURL(),
@@ -568,7 +580,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active"
     );
   });
@@ -581,7 +595,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #888 50%, #888 50%)"]`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-prefix .prefix-span[style="background: linear-gradient(90deg, #888 50%, #888 50%)"]`
       ),
       "category1 section link is rendered with the right solid prefix icon color"
     );
@@ -600,7 +614,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     await visit("/");
 
     assert.strictEqual(
-      query(`.sidebar-section-link-${category.slug}`).title,
+      query(`.sidebar-section-link[data-category-id="${category.id}"]`).title,
       category.description_text,
       "category description without HTML entity is used as the link's title"
     );
@@ -618,7 +632,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active for the new route"
     );
   });
@@ -635,7 +651,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active for the unread route"
     );
   });
@@ -652,7 +670,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active for the top route"
     );
   });
@@ -669,7 +689,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active for the none route"
     );
   });
@@ -686,7 +708,9 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
     );
 
     assert.ok(
-      exists(`.sidebar-section-link-${category1.slug}.active`),
+      exists(
+        `.sidebar-section-link[data-category-id="${category1.id}"].active`
+      ),
       "the category1 section link is marked as active for the all route"
     );
   });
@@ -725,7 +749,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-suffix`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-suffix`
       ),
       "shows suffix indicator for unread content on categories link"
     );
@@ -741,7 +765,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-suffix`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-suffix`
       ),
       "shows suffix indicator for new topics on categories link"
     );
@@ -757,7 +781,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       !exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-suffix`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-suffix`
       ),
       "hides suffix indicator when there's no new/unread content on category link"
     );
@@ -817,7 +841,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.strictEqual(
       query(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-content-badge`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-content-badge`
       ).textContent.trim(),
       I18n.t("sidebar.unread_count", { count: 1 }),
       `displays 1 unread count for ${category1.slug} section link`
@@ -825,7 +849,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.strictEqual(
       query(
-        `.sidebar-section-link-${category2.slug} .sidebar-section-link-content-badge`
+        `.sidebar-section-link[data-category-id="${category2.id}"] .sidebar-section-link-content-badge`
       ).textContent.trim(),
       I18n.t("sidebar.unread_count", { count: 2 }),
       `displays 2 unread count for ${category2.slug} section link`
@@ -842,7 +866,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.strictEqual(
       query(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-content-badge`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-content-badge`
       ).textContent.trim(),
       I18n.t("sidebar.new_count", { count: 1 }),
       `displays 1 new count for ${category1.slug} section link`
@@ -859,7 +883,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.ok(
       !exists(
-        `.sidebar-section-link-${category1.slug} .sidebar-section-link-content-badge`
+        `.sidebar-section-link[data-category-id="${category1.id}"] .sidebar-section-link-content-badge`
       ),
       `does not display any badge ${category1.slug} section link`
     );
@@ -875,7 +899,7 @@ acceptance("Sidebar - Logged on user - Categories Section", function (needs) {
 
     assert.strictEqual(
       query(
-        `.sidebar-section-link-${category2.slug} .sidebar-section-link-content-badge`
+        `.sidebar-section-link[data-category-id="${category2.id}"] .sidebar-section-link-content-badge`
       ).textContent.trim(),
       I18n.t("sidebar.unread_count", { count: 1 }),
       `displays 1 unread count for ${category2.slug} section link`
diff --git a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js
index 1414abe8fb9..4ac39be1264 100644
--- a/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js
+++ b/app/assets/javascripts/discourse/tests/acceptance/user-preferences-sidebar-test.js
@@ -66,7 +66,9 @@ acceptance("User Preferences - Sidebar", function (needs) {
     await visit("/");
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-support"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=6]"
+      ),
       "support category is present in sidebar"
     );
 
@@ -90,12 +92,16 @@ acceptance("User Preferences - Sidebar", function (needs) {
     await click(".dialog-footer .btn-primary");
 
     assert.ok(
-      !exists(".sidebar-section-categories .sidebar-section-link-howto"),
+      !exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=10]"
+      ),
       "howto category is not displayed in sidebar"
     );
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-support"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=6]"
+      ),
       "support category is displayed in sidebar"
     );
   });
@@ -113,12 +119,16 @@ acceptance("User Preferences - Sidebar", function (needs) {
     await click(".save-changes");
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-support"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=6]"
+      ),
       "support category has been added to sidebar"
     );
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-bug"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=1]"
+      ),
       "bug category has been added to sidebar"
     );
   });
@@ -137,12 +147,16 @@ acceptance("User Preferences - Sidebar", function (needs) {
     await click(".save-changes");
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-support"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=6]"
+      ),
       "support category has been added to sidebar"
     );
 
     assert.ok(
-      exists(".sidebar-section-categories .sidebar-section-link-bug"),
+      exists(
+        ".sidebar-section-categories .sidebar-section-link[data-category-id=1]"
+      ),
       "bug category has been added to sidebar"
     );