UX: Improvements to user tips (#26480)

- Add a "Skip tips" button to first notification tip
- Add a "Skip tips" button to the admin guide tip
- Fixes the timeline tip showing when no timeline was present
- Fixes post menu tip showing when no "..." button is present
- Adds system tests
- Marks each tip as seen as soon as it is displayed so that refreshing,
clicking outside, etc. won't show it again
- Change just above means we no longer need a MessageBus track

Co-authored-by: Bianca Nenciu <nbianca@users.noreply.github.com>

app/assets/javascripts/discourse/app/components/bootstrap-mode-notice.hbs

@@ -11,6 +11,7 @@
           @priority={{900}}
           @titleText={{i18n "user_tips.admin_guide.title"}}
           @contentHtml={{this.userTipContent}}
+          @showSkipButton={{true}}
         />
       {{/if}}
     </DButton>

app/assets/javascripts/discourse/app/components/header/user-dropdown/notifications.gjs

@@ -54,6 +54,7 @@ export default class Notifications extends Component {
         @placement="bottom-end"
         @titleText={{i18n "user_tips.first_notification.title"}}
         @contentText={{i18n "user_tips.first_notification.content"}}
+        @showSkipButton={{true}}
       />
     {{/if}}
 

app/assets/javascripts/discourse/app/components/topic-timeline.hbs

@@ -1,11 +1,3 @@
-<UserTip
-  @id="topic_timeline"
-  @titleText={{i18n "user_tips.topic_timeline.title"}}
-  @contentText={{i18n "user_tips.topic_timeline.content"}}
-  @placement="left"
-  @triggerSelector=".timeline-scrollarea-wrapper"
-/>
-
 <div
   class={{concat-class "timeline-container" this.classes}}
   {{did-insert this.addShowClass}}

app/assets/javascripts/discourse/app/components/topic-timeline/container.hbs

@@ -61,6 +61,14 @@
 {{/if}}
 
 {{#if this.displayTimeLineScrollArea}}
+  <UserTip
+    @id="topic_timeline"
+    @titleText={{i18n "user_tips.topic_timeline.title"}}
+    @contentText={{i18n "user_tips.topic_timeline.content"}}
+    @placement="left"
+    @triggerSelector=".timeline-scrollarea-wrapper"
+  />
+
   <div class="timeline-scrollarea-wrapper">
     <div class="timeline-date-wrapper">
       <a

app/assets/javascripts/discourse/app/components/user-tip-container.gjs

@@ -11,6 +11,10 @@ export default class UserTipContainer extends Component {
     return htmlSafe(this.args.data.contentHtml);
   }
 
+  get showSkipButton() {
+    return this.args.data.showSkipButton;
+  }
+
   @action
   handleDismiss(_, event) {
     event.preventDefault();
@@ -18,6 +22,13 @@ export default class UserTipContainer extends Component {
     this.userTips.hideUserTipForever(this.args.data.id);
   }
 
+  @action
+  handleSkip(_, event) {
+    event.preventDefault();
+    this.args.close();
+    this.userTips.skipTips();
+  }
+
   <template>
     <div class="user-tip__container">
       <div class="user-tip__title">{{@data.titleText}}</div>
@@ -28,16 +39,23 @@ export default class UserTipContainer extends Component {
           {{@data.contentText}}
         {{/if}}
       </div>
-      {{#if @data.onDismiss}}
-        <div class="user-tip__buttons">
+      <div class="user-tip__buttons">
+        <DButton
+          class="btn-primary"
+          @translatedLabel={{@data.buttonText}}
+          @action={{this.handleDismiss}}
+          @forwardEvent={{true}}
+        />
+
+        {{#if this.showSkipButton}}
           <DButton
-            class="btn-primary"
-            @translatedLabel={{@data.buttonText}}
-            @action={{this.handleDismiss}}
+            class="btn-flat btn-text"
+            @translatedLabel={{@data.buttonSkipText}}
+            @action={{this.handleSkip}}
             @forwardEvent={{true}}
           />
-        </div>
-      {{/if}}
+        {{/if}}
+      </div>
     </div>
   </template>
 }

app/assets/javascripts/discourse/app/components/user-tip.gjs

@@ -55,14 +55,19 @@ export default class UserTip extends Component {
           contentText: this.args.contentText
             ? escape(this.args.contentText)
             : null,
-          onDismiss: () => {
-            this.userTips.hideUserTipForever(this.args.id);
-          },
           buttonText,
+          buttonSkipText: I18n.t("user_tips.skip"),
+          showSkipButton: this.args.showSkipButton,
         },
       });
 
       this.tooltip.show(instance);
+
+      if (this.shouldRenderTip) {
+        // mark tooltip directly as seen so that
+        // refreshing, clicking outside, etc. won't show it again
+        this.userTips.markAsSeen(this.args.id);
+      }
     });
 
     return () => {

app/assets/javascripts/discourse/app/instance-initializers/user-tips.js

@@ -1,51 +0,0 @@
-import { bind } from "discourse-common/utils/decorators";
-
-export default {
-  after: "message-bus",
-
-  initialize(owner) {
-    this.currentUser = owner.lookup("service:current-user");
-    if (!this.currentUser) {
-      return;
-    }
-
-    this.userTips = owner.lookup("service:user-tips");
-    this.messageBus = owner.lookup("service:message-bus");
-    this.site = owner.lookup("service:site");
-
-    this.messageBus.subscribe(
-      `/user-tips/${this.currentUser.id}`,
-      this.onMessage
-    );
-  },
-
-  teardown() {
-    if (this.currentUser) {
-      this.messageBus?.unsubscribe(
-        `/user-tips/${this.currentUser.id}`,
-        this.onMessage
-      );
-    }
-  },
-
-  @bind
-  onMessage(seenUserTips) {
-    if (!this.site.user_tips) {
-      return;
-    }
-
-    if (!this.currentUser.user_option) {
-      this.currentUser.set("user_option", {});
-    }
-
-    this.currentUser.set("user_option.seen_popups", seenUserTips);
-
-    (seenUserTips || []).forEach((userTipId) => {
-      this.userTips.hideUserTipForever(
-        Object.keys(this.site.user_tips).find(
-          (id) => this.site.user_tips[id] === userTipId
-        )
-      );
-    });
-  },
-};

app/assets/javascripts/discourse/app/services/user-tips.js

@@ -39,8 +39,10 @@ export default class UserTips extends Service {
   }
 
   addAvailableTip(tip) {
-    this.#availableTips.add(tip);
-    this.#updateRenderedId();
+    if (this.canSeeUserTip(tip.id) && !this._findAvailableTipById(tip.id)) {
+      this.#availableTips.add(tip);
+      this.#updateRenderedId();
+    }
   }
 
   removeAvailableTip(tip) {
@@ -84,7 +86,6 @@ export default class UserTips extends Service {
       return;
     }
 
-    // Empty tipId means all user tips.
     if (!userTips[tipId]) {
       // eslint-disable-next-line no-console
       console.warn("Cannot hide user tip with id", tipId);
@@ -94,6 +95,23 @@ export default class UserTips extends Service {
     const tipObj = [...this.#availableTips].find((t) => t.id === tipId);
     this.removeAvailableTip(tipObj);
 
+    await this.markAsSeen(tipId);
+  }
+
+  async markAsSeen(tipId) {
+    if (!this.currentUser) {
+      return;
+    }
+
+    if (!tipId) {
+      return;
+    }
+
+    const userTips = this.site.user_tips;
+    if (!userTips || this.currentUser.user_option?.skip_new_user_tips) {
+      return;
+    }
+
     // Update list of seen user tips.
     let seenUserTips = this.currentUser.user_option?.seen_popups || [];
     if (seenUserTips.includes(userTips[tipId])) {
@@ -108,4 +126,25 @@ export default class UserTips extends Service {
     this.currentUser.set("user_option.seen_popups", seenUserTips);
     await this.currentUser.save(["seen_popups"]);
   }
+
+  async skipTips() {
+    if (!this.currentUser) {
+      return;
+    }
+
+    this.#availableTips.clear();
+    this.#shouldRenderSet.clear();
+
+    this.currentUser.set("user_option.skip_new_user_tips", true);
+    await this.currentUser.save(["skip_new_user_tips"]);
+  }
+
+  _findAvailableTipById(id) {
+    for (let item of this.#availableTips) {
+      if (item.id === id) {
+        return item;
+      }
+    }
+    return null;
+  }
 }

app/assets/javascripts/discourse/app/widgets/header-user-tip-shim.js

@@ -4,5 +4,5 @@ import { registerWidgetShim } from "discourse/widgets/render-glimmer";
 registerWidgetShim(
   "header-user-tip-shim",
   "div.header-user-tip-shim",
-  hbs`<UserTip @id="first_notification" @triggerSelector=".header-dropdown-toggle.current-user" @placement="bottom-end" @titleText={{i18n "user_tips.first_notification.title"}} @contentText={{i18n "user_tips.first_notification.content"}} />`
+  hbs`<UserTip @id="first_notification" @triggerSelector=".header-dropdown-toggle.current-user" @placement="bottom-end" @titleText={{i18n "user_tips.first_notification.title"}} @contentText={{i18n "user_tips.first_notification.content"}} @showSkipButton={{true}} />`
 );

app/assets/javascripts/discourse/app/widgets/post-menu.js

@@ -621,6 +621,7 @@ export default createWidget("post-menu", {
       visibleButtons = allButtons;
     }
 
+    let hasShowMoreButton = false;
     // Only show ellipsis if there is more than one button hidden
     // if there are no more buttons, we are not collapsed
     if (!state.collapsed || allButtons.length <= visibleButtons.length + 1) {
@@ -636,6 +637,7 @@ export default createWidget("post-menu", {
         icon: "ellipsis-h",
       });
       visibleButtons.splice(visibleButtons.length - 1, 0, showMore);
+      hasShowMoreButton = true;
     }
 
     Object.values(_extraButtons).forEach((builder) => {
@@ -813,7 +815,9 @@ export default createWidget("post-menu", {
         })
       );
     }
-
+    if (hasShowMoreButton) {
+      contents.push(this.attach("post-user-tip-shim"));
+    }
     return contents;
   },
 

app/assets/javascripts/discourse/app/widgets/post.js

@@ -1009,10 +1009,7 @@ export default createWidget("post", {
       return "";
     }
 
-    return [
-      this.attach("post-user-tip-shim"),
-      this.attach("post-article", attrs),
-    ];
+    return [this.attach("post-article", attrs)];
   },
 
   toggleLike() {

app/assets/stylesheets/common/base/user-tips.scss

@@ -29,10 +29,17 @@
 
   .user-tip__buttons {
     margin-top: 1em;
+    display: flex;
+    justify-content: space-between;
   }
 
   .btn-primary {
     background: var(--secondary);
     color: var(--tertiary);
   }
+
+  .btn-flat.btn-text {
+    background-color: transparent;
+    color: var(--secondary);
+  }
 }

app/assets/stylesheets/common/float-kit/d-tooltip.scss

@@ -64,28 +64,34 @@
 
   &[data-placement^="top"] {
     .arrow {
-      bottom: -9px;
+      bottom: -10px;
       rotate: 180deg;
     }
   }
 
+  &[data-placement^="top-start"] {
+    .arrow {
+      margin-left: 10px;
+    }
+  }
+
   &[data-placement^="bottom"] {
     .arrow {
-      top: -9px;
+      top: -10px;
     }
   }
 
   &[data-placement^="right"] {
     .arrow {
       rotate: -90deg;
-      left: -9px;
+      left: -10px;
     }
   }
 
   &[data-placement^="left"] {
     .arrow {
       rotate: 90deg;
-      right: -9px;
+      right: -10px;
     }
   }
 }

app/services/user_updater.rb

@@ -265,13 +265,6 @@ class UserUpdater
           user_notification_schedule.destroy_scheduled_timings
         end
       end
-      if attributes.key?(:seen_popups) || attributes.key?(:skip_new_user_tips)
-        MessageBus.publish(
-          "/user-tips/#{user.id}",
-          user.user_option.seen_popups,
-          user_ids: [user.id],
-        )
-      end
       DiscourseEvent.trigger(:user_updated, user)
     end
 

config/locales/client.en.yml

@@ -1990,6 +1990,7 @@ en:
 
     user_tips:
       button: "Got it!"
+      skip: "Skip tips"
 
       first_notification:
         title: "Your first notification!"

spec/services/user_updater_spec.rb

@@ -586,14 +586,10 @@ RSpec.describe UserUpdater do
 
     context "when skip_new_user_tips is edited" do
       it "updates seen_popups too" do
-        messages =
-          MessageBus.track_publish("/user-tips/#{user.id}") do
-            UserUpdater.new(Discourse.system_user, user).update(skip_new_user_tips: true)
-          end
+        UserUpdater.new(Discourse.system_user, user).update(skip_new_user_tips: true)
 
         expect(user.user_option.skip_new_user_tips).to eq(true)
         expect(user.user_option.seen_popups).to eq([-1])
-        expect(messages.map(&:data)).to contain_exactly([-1])
       end
 
       it "does not reset seen_popups" do
@@ -606,18 +602,6 @@ RSpec.describe UserUpdater do
       end
     end
 
-    context "when seen_popups is edited" do
-      it "publishes a message" do
-        messages =
-          MessageBus.track_publish("/user-tips/#{user.id}") do
-            UserUpdater.new(Discourse.system_user, user).update(seen_popups: [1])
-          end
-
-        expect(user.user_option.seen_popups).to eq([1])
-        expect(messages.map(&:data)).to contain_exactly([1])
-      end
-    end
-
     it "logs the action" do
       user = Fabricate(:user, name: "Billy Bob")
 

spec/system/user_tips_spec.rb

@@ -0,0 +1,84 @@
+# frozen_string_literal: true
+
+describe "Homepage", type: :system do
+  fab!(:admin)
+  fab!(:user)
+  fab!(:topics) { Fabricate.times(2, :post).map(&:topic) }
+  fab!(:posts) { Fabricate.times(3, :post, topic: topics[0]) }
+  let(:discovery) { PageObjects::Pages::Discovery.new }
+
+  context "when user tips are disabled" do
+    before { SiteSetting.enable_user_tips = false }
+
+    it "does not show the 'first notification' tip to the user when disabled" do
+      sign_in user
+      visit "/"
+
+      expect(page).to have_no_css(".fk-d-tooltip .user-tip__title")
+    end
+
+    it "does not show the boostrapping tip to an admin user" do
+      SiteSetting.bootstrap_mode_enabled = true
+      sign_in admin
+      visit "/"
+
+      expect(page).to have_no_css(".fk-d-tooltip .user-tip__title")
+    end
+  end
+
+  context "when user tips are enabled" do
+    before { SiteSetting.enable_user_tips = true }
+
+    it "shows the 'first notification' tip to the user when enabled" do
+      sign_in user
+      expect(user.user_option.seen_popups).to eq(nil)
+
+      visit "/"
+
+      expect(page).to have_css(".fk-d-tooltip .user-tip__title", text: "Your first notification!")
+
+      # refresh the page
+      visit "/"
+      # Tip is marked as seen when it is displayed
+      expect(user.reload.user_option.seen_popups).to include(User.user_tips[:first_notification])
+      expect(page).to have_no_css(
+        ".fk-d-tooltip .user-tip__title",
+        text: "Your first notification!",
+      )
+    end
+
+    it "shows the boostrapping tip to an admin user" do
+      SiteSetting.bootstrap_mode_enabled = true
+      expect(admin.user_option.seen_popups).to eq(nil)
+      sign_in admin
+      visit "/"
+
+      expect(page).to have_css(".fk-d-tooltip .user-tip__title", text: "Welcome to your new site!")
+    end
+
+    it "shows a second notification once first is dismissed and user visits a topic" do
+      sign_in user
+      visit "/"
+
+      find(".fk-d-tooltip .user-tip__buttons .btn-primary").click
+      expect(page).to have_no_css(".fk-d-tooltip .user-tip__title")
+
+      discovery.topic_list.visit_topic(topics[0])
+      expect(page).to have_css(".fk-d-tooltip .user-tip__title", text: "Topic timeline")
+
+      find(".fk-d-tooltip .user-tip__buttons .btn-primary").click
+      expect(page).to have_css(".fk-d-tooltip .user-tip__title", text: "Keep reading!")
+    end
+
+    it "can skip all tips" do
+      sign_in user
+      visit "/"
+
+      find(".fk-d-tooltip .user-tip__buttons .btn", text: "Skip tips").click
+      expect(page).to have_no_css(".fk-d-tooltip .user-tip__title")
+
+      discovery.topic_list.visit_topic(topics[0])
+      expect(page).to have_no_css(".fk-d-tooltip .user-tip__title")
+    end
+  end
+end