From 2a49757f35b1f503624cc2cfc44b01d8ba45519c Mon Sep 17 00:00:00 2001
From: Kris <kris.aubuchon@discourse.org>
Date: Sun, 20 Aug 2023 21:34:12 -0400
Subject: [PATCH] UX: update `/new` toggle styles, class names (#23154)

* Minor style adjustments
* Removes "all" count because it's redundant to the count on New
* Updates generic class names with -- modifier to follow BEM and help avoid class name collisions
* Hides the toggle when bulk select is enabled (the UI ends up being too busy)
---
 .../new-list-header-controls-wrapper.js       |  6 +++---
 .../discourse/app/components/topic-list.js    |  6 +++---
 .../list/new-list-header-controls.hbr         |  8 +++----
 .../topic-list-header-column.hbr              | 12 ++++++-----
 .../list/new-list-header-controls.js          | 10 ---------
 .../app/templates/mobile/discovery/topics.hbs | 19 +++++++++--------
 .../discourse/app/templates/tag/show.hbs      | 21 +++++++++++--------
 .../stylesheets/desktop/topic-list.scss       |  5 +++--
 spec/system/new_topic_list_spec.rb            | 18 ----------------
 .../components/new_topic_list_toggle.rb       |  6 +++---
 10 files changed, 45 insertions(+), 66 deletions(-)

diff --git a/app/assets/javascripts/discourse/app/components/new-list-header-controls-wrapper.js b/app/assets/javascripts/discourse/app/components/new-list-header-controls-wrapper.js
index 68a6b7b7df8..ff2cc7d27bf 100644
--- a/app/assets/javascripts/discourse/app/components/new-list-header-controls-wrapper.js
+++ b/app/assets/javascripts/discourse/app/components/new-list-header-controls-wrapper.js
@@ -3,11 +3,11 @@ import Component from "@glimmer/component";
 export default class NewListHeaderControlsWrapper extends Component {
   click(e) {
     const target = e.target;
-    if (target.closest("button.topics-replies-toggle.all")) {
+    if (target.closest("button.topics-replies-toggle.--all")) {
       this.args.changeNewListSubset(null);
-    } else if (target.closest("button.topics-replies-toggle.topics")) {
+    } else if (target.closest("button.topics-replies-toggle.--topics")) {
       this.args.changeNewListSubset("topics");
-    } else if (target.closest("button.topics-replies-toggle.replies")) {
+    } else if (target.closest("button.topics-replies-toggle.--replies")) {
       this.args.changeNewListSubset("replies");
     }
   }
diff --git a/app/assets/javascripts/discourse/app/components/topic-list.js b/app/assets/javascripts/discourse/app/components/topic-list.js
index 5a2f6b1078a..e6f5cb88fcb 100644
--- a/app/assets/javascripts/discourse/app/components/topic-list.js
+++ b/app/assets/javascripts/discourse/app/components/topic-list.js
@@ -186,11 +186,11 @@ export default Component.extend(LoadMore, {
     });
 
     onClick("button.topics-replies-toggle", (element) => {
-      if (element.classList.contains("all")) {
+      if (element.classList.contains("--all")) {
         this.changeNewListSubset(null);
-      } else if (element.classList.contains("topics")) {
+      } else if (element.classList.contains("--topics")) {
         this.changeNewListSubset("topics");
-      } else if (element.classList.contains("replies")) {
+      } else if (element.classList.contains("--replies")) {
         this.changeNewListSubset("replies");
       }
       this.rerender();
diff --git a/app/assets/javascripts/discourse/app/raw-templates/list/new-list-header-controls.hbr b/app/assets/javascripts/discourse/app/raw-templates/list/new-list-header-controls.hbr
index 86c944165f6..9757caf8c30 100644
--- a/app/assets/javascripts/discourse/app/raw-templates/list/new-list-header-controls.hbr
+++ b/app/assets/javascripts/discourse/app/raw-templates/list/new-list-header-controls.hbr
@@ -1,13 +1,13 @@
 {{#if view.staticLabel}}
   <span class="static-label">{{view.staticLabel}}</span>
 {{else}}
-  <button class="topics-replies-toggle all{{#if view.allActive}} active{{/if}}">
-    {{view.allButtonLabel}}
+  <button class="topics-replies-toggle --all{{#if view.allActive}} active{{/if}}">
+    {{i18n "filters.new.all"}}
   </button>
-  <button class="topics-replies-toggle topics{{#if view.topicsActive}} active{{/if}}">
+  <button class="topics-replies-toggle --topics{{#if view.topicsActive}} active{{/if}}">
     {{view.topicsButtonLabel}}
   </button>
-  <button class="topics-replies-toggle replies{{#if view.repliesActive}} active{{/if}}">
+  <button class="topics-replies-toggle --replies{{#if view.repliesActive}} active{{/if}}">
     {{view.repliesButtonLabel}}
   </button>
 {{/if}}
diff --git a/app/assets/javascripts/discourse/app/raw-templates/topic-list-header-column.hbr b/app/assets/javascripts/discourse/app/raw-templates/topic-list-header-column.hbr
index 60176b0c627..59f374c333b 100644
--- a/app/assets/javascripts/discourse/app/raw-templates/topic-list-header-column.hbr
+++ b/app/assets/javascripts/discourse/app/raw-templates/topic-list-header-column.hbr
@@ -13,11 +13,13 @@
       </span>
     {{/if ~}}
   {{/if ~}}
-  {{~#if view.showTopicsAndRepliesToggle}}
-    {{raw "list/new-list-header-controls" current=newListSubset newRepliesCount=newRepliesCount newTopicsCount=newTopicsCount}}
-  {{else}}
-    <span>{{view.localizedName}}</span>
-  {{/if ~}}
+  {{~#unless bulkSelectEnabled}}
+    {{~#if view.showTopicsAndRepliesToggle}}
+      {{raw "list/new-list-header-controls" current=newListSubset newRepliesCount=newRepliesCount newTopicsCount=newTopicsCount}}
+    {{else}}
+      <span>{{view.localizedName}}</span>
+    {{/if ~}}
+  {{/unless ~}}
   {{~#if view.isSorting}}
     {{d-icon view.sortIcon}}
   {{/if ~}}
diff --git a/app/assets/javascripts/discourse/app/raw-views/list/new-list-header-controls.js b/app/assets/javascripts/discourse/app/raw-views/list/new-list-header-controls.js
index 541d94759ba..7a6a84d3e8d 100644
--- a/app/assets/javascripts/discourse/app/raw-views/list/new-list-header-controls.js
+++ b/app/assets/javascripts/discourse/app/raw-views/list/new-list-header-controls.js
@@ -18,16 +18,6 @@ export default EmberObject.extend({
     return !this.topicsActive && !this.repliesActive;
   },
 
-  @discourseComputed
-  allButtonLabel() {
-    const count = this.newRepliesCount + this.newTopicsCount;
-    if (count > 0) {
-      return I18n.t("filters.new.all_with_count", { count });
-    } else {
-      return I18n.t("filters.new.all");
-    }
-  },
-
   @discourseComputed
   repliesButtonLabel() {
     if (this.newRepliesCount > 0) {
diff --git a/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs b/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs
index f8e33f7bca1..d3d51bc9749 100644
--- a/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs
+++ b/app/assets/javascripts/discourse/app/templates/mobile/discovery/topics.hbs
@@ -39,15 +39,16 @@
       </a>
     {{/if}}
   {{/if}}
-
-  {{#if this.showTopicsAndRepliesToggle}}
-    <NewListHeaderControlsWrapper
-      @current={{this.model.params.subset}}
-      @newRepliesCount={{this.newRepliesCount}}
-      @newTopicsCount={{this.newTopicsCount}}
-      @changeNewListSubset={{route-action "changeNewListSubset"}}
-    />
-  {{/if}}
+  {{#unless this.bulkSelectEnabled}}
+    {{#if this.showTopicsAndRepliesToggle}}
+      <NewListHeaderControlsWrapper
+        @current={{this.model.params.subset}}
+        @newRepliesCount={{this.newRepliesCount}}
+        @newTopicsCount={{this.newTopicsCount}}
+        @changeNewListSubset={{route-action "changeNewListSubset"}}
+      />
+    {{/if}}
+  {{/unless}}
   {{#if this.hasTopics}}
     <TopicList
       @ascending={{this.ascending}}
diff --git a/app/assets/javascripts/discourse/app/templates/tag/show.hbs b/app/assets/javascripts/discourse/app/templates/tag/show.hbs
index c5f1e0fa585..6c3750fd2fa 100644
--- a/app/assets/javascripts/discourse/app/templates/tag/show.hbs
+++ b/app/assets/javascripts/discourse/app/templates/tag/show.hbs
@@ -126,15 +126,18 @@
                   </div>
                 {{/if}}
               {{/if}}
-
-              {{#if (and this.showTopicsAndRepliesToggle this.site.mobileView)}}
-                <NewListHeaderControlsWrapper
-                  @current={{this.subset}}
-                  @newRepliesCount={{this.newRepliesCount}}
-                  @newTopicsCount={{this.newTopicsCount}}
-                  @changeNewListSubset={{action "changeNewListSubset"}}
-                />
-              {{/if}}
+              {{#unless this.bulkSelectEnabled}}
+                {{#if
+                  (and this.showTopicsAndRepliesToggle this.site.mobileView)
+                }}
+                  <NewListHeaderControlsWrapper
+                    @current={{this.subset}}
+                    @newRepliesCount={{this.newRepliesCount}}
+                    @newTopicsCount={{this.newTopicsCount}}
+                    @changeNewListSubset={{action "changeNewListSubset"}}
+                  />
+                {{/if}}
+              {{/unless}}
               {{#if this.list.topics}}
                 <TopicList
                   @topics={{this.list.topics}}
diff --git a/app/assets/stylesheets/desktop/topic-list.scss b/app/assets/stylesheets/desktop/topic-list.scss
index c660c1f52f9..65b0a8851cc 100644
--- a/app/assets/stylesheets/desktop/topic-list.scss
+++ b/app/assets/stylesheets/desktop/topic-list.scss
@@ -39,6 +39,7 @@
 
   button.bulk-select {
     padding: 0;
+    margin-right: 0.5em;
     line-height: var(--line-height-large);
   }
 
@@ -59,8 +60,8 @@
     background: none;
     border: none;
     line-height: var(--line-height-large);
-    min-height: 30px;
-
+    padding-left: 0.5em;
+    padding-right: 0.5em;
     &.active {
       background: var(--quaternary);
       color: var(--secondary);
diff --git a/spec/system/new_topic_list_spec.rb b/spec/system/new_topic_list_spec.rb
index 3fe2d744a89..6901b18bff7 100644
--- a/spec/system/new_topic_list_spec.rb
+++ b/spec/system/new_topic_list_spec.rb
@@ -88,7 +88,6 @@ describe "New topic list", type: :system do
           new_topic_with_tag,
         ].each { |topic| expect(topic_list).to have_topic(topic) }
 
-        expect(tabs_toggle.all_tab).to have_count(6)
         expect(tabs_toggle.replies_tab).to have_count(3)
         expect(tabs_toggle.topics_tab).to have_count(3)
 
@@ -124,7 +123,6 @@ describe "New topic list", type: :system do
         expect(tabs_toggle.replies_tab).to be_inactive
         expect(tabs_toggle.topics_tab).to be_active
 
-        expect(tabs_toggle.all_tab).to have_count(6)
         expect(tabs_toggle.replies_tab).to have_count(3)
         expect(tabs_toggle.topics_tab).to have_count(3)
 
@@ -144,7 +142,6 @@ describe "New topic list", type: :system do
         expect(tabs_toggle.replies_tab).to be_active
         expect(tabs_toggle.topics_tab).to be_inactive
 
-        expect(tabs_toggle.all_tab).to have_count(6)
         expect(tabs_toggle.replies_tab).to have_count(3)
         expect(tabs_toggle.topics_tab).to have_count(3)
 
@@ -170,19 +167,16 @@ describe "New topic list", type: :system do
       it "live-updates the counts shown on the tabs" do
         visit("/new")
 
-        expect(tabs_toggle.all_tab).to have_count(6)
         expect(tabs_toggle.replies_tab).to have_count(3)
         expect(tabs_toggle.topics_tab).to have_count(3)
 
         TopicUser.update_last_read(user, new_reply_in_category.id, 2, 1, 1)
 
-        expect(tabs_toggle.all_tab).to have_count(5)
         expect(tabs_toggle.replies_tab).to have_count(2)
         expect(tabs_toggle.topics_tab).to have_count(3)
 
         TopicUser.update_last_read(user, new_topic.id, 1, 1, 1)
 
-        expect(tabs_toggle.all_tab).to have_count(4)
         expect(tabs_toggle.replies_tab).to have_count(2)
         expect(tabs_toggle.topics_tab).to have_count(2)
       end
@@ -198,7 +192,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_inactive
           expect(tabs_toggle.topics_tab).to be_inactive
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
         end
@@ -214,7 +207,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_inactive
           expect(tabs_toggle.topics_tab).to be_active
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
 
@@ -234,7 +226,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_active
           expect(tabs_toggle.topics_tab).to be_inactive
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
 
@@ -262,13 +253,11 @@ describe "New topic list", type: :system do
 
           visit("/c/#{category.slug}/#{category.id}/l/new")
 
-          expect(tabs_toggle.all_tab).to have_count(3)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(2)
 
           TopicUser.update_last_read(user, new_topic_in_category.id, 1, 1, 1)
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
         end
@@ -286,7 +275,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_inactive
           expect(tabs_toggle.topics_tab).to be_inactive
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
         end
@@ -302,7 +290,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_inactive
           expect(tabs_toggle.topics_tab).to be_active
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
 
@@ -321,7 +308,6 @@ describe "New topic list", type: :system do
           expect(tabs_toggle.replies_tab).to be_active
           expect(tabs_toggle.topics_tab).to be_inactive
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
 
@@ -347,13 +333,11 @@ describe "New topic list", type: :system do
 
           visit("/tag/#{tag.name}/l/new")
 
-          expect(tabs_toggle.all_tab).to have_count(3)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(2)
 
           TopicUser.update_last_read(user, new_topic_with_tag.id, 1, 1, 1)
 
-          expect(tabs_toggle.all_tab).to have_count(2)
           expect(tabs_toggle.replies_tab).to have_count(1)
           expect(tabs_toggle.topics_tab).to have_count(1)
         end
@@ -389,7 +373,6 @@ describe "New topic list", type: :system do
         expect(tabs_toggle.replies_tab).to be_visible
         expect(tabs_toggle.topics_tab).to be_visible
 
-        expect(tabs_toggle.all_tab).to have_count(3)
         expect(tabs_toggle.replies_tab).to have_count(3)
         expect(tabs_toggle.topics_tab).to have_count(0)
       end
@@ -411,7 +394,6 @@ describe "New topic list", type: :system do
         expect(tabs_toggle.replies_tab).to be_visible
         expect(tabs_toggle.topics_tab).to be_visible
 
-        expect(tabs_toggle.all_tab).to have_count(3)
         expect(tabs_toggle.replies_tab).to have_count(0)
         expect(tabs_toggle.topics_tab).to have_count(3)
       end
diff --git a/spec/system/page_objects/components/new_topic_list_toggle.rb b/spec/system/page_objects/components/new_topic_list_toggle.rb
index 2bce999fee4..fd93d0367e7 100644
--- a/spec/system/page_objects/components/new_topic_list_toggle.rb
+++ b/spec/system/page_objects/components/new_topic_list_toggle.rb
@@ -5,9 +5,9 @@ module PageObjects
     class NewTopicListToggle < PageObjects::Components::Base
       COMMON_SELECTOR = ".topics-replies-toggle"
 
-      ALL_SELECTOR = "#{COMMON_SELECTOR}.all"
-      REPLIES_SELECTOR = "#{COMMON_SELECTOR}.replies"
-      TOPICS_SELECTOR = "#{COMMON_SELECTOR}.topics"
+      ALL_SELECTOR = "#{COMMON_SELECTOR}.--all"
+      REPLIES_SELECTOR = "#{COMMON_SELECTOR}.--replies"
+      TOPICS_SELECTOR = "#{COMMON_SELECTOR}.--topics"
 
       def not_rendered?
         has_no_css?(COMMON_SELECTOR)