DEV: Refactor community section code (#18436)

In a recent commit when adding the review section link, I moved to a pattern where we allowed the section links to be refreshed after the section has been constructed. However, we were not tearing down the old section links when refreshing. This made me realise that refreshing section links in a section is not a pattern I want to adopt since people can easily forget to teardown. Instead, each section link should be responsible for defining a teardown function for cleanup which will always be called when the sidebar is removed.
2022-09-30 13:13:50 +08:00 · 2022-09-30 13:13:50 +08:00 · 0bdb616edc
parent 079450c9e4
commit 0bdb616edc
8 changed files with 140 additions and 125 deletions
--- a/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.hbs
@ -13,6 +13,7 @@

  {{#each this.sectionLinks as |sectionLink|}}
    <Sidebar::SectionLink
+      @shouldDisplay={{sectionLink.shouldDisplay}}
      @linkName={{sectionLink.name}}
      @href={{sectionLink.href}}
      @route={{sectionLink.route}}
--- a/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js
+++ b/app/assets/javascripts/discourse/app/components/sidebar/common/community-section.js
@ -25,7 +25,23 @@ export default class SidebarCommunitySection extends Component {
  constructor() {
    super(...arguments);

-    this.refreshSectionLinks();
+    this.moreSectionLinks = this.#initializeSectionLinks(
+      [...this.defaultMoreSectionLinks, ...customSectionLinks],
+      { inMoreDrawer: true }
+    );
+
+    this.moreSecondarySectionLinks = this.#initializeSectionLinks(
+      [
+        ...this.defaultMoreSecondarySectionLinks,
+        ...secondaryCustomSectionLinks,
+      ],
+      { inMoreDrawer: true }
+    );
+
+    this.sectionLinks = this.#initializeSectionLinks(
+      this.defaultMainSectionLinks,
+      { inMoreDrawer: false }
+    );

    this.callbackId = this.topicTrackingState.onStateChange(() => {
      this.sectionLinks.forEach((sectionLink) => {
@ -61,41 +77,20 @@ export default class SidebarCommunitySection extends Component {
    return [];
  }

-  refreshSectionLinks() {
-    this.moreSectionLinks = this.#initializeSectionLinks([
-      ...this.defaultMoreSectionLinks,
-      ...customSectionLinks,
-    ]);
-
-    this.moreSecondarySectionLinks = this.#initializeSectionLinks([
-      ...this.defaultMoreSecondarySectionLinks,
-      ...secondaryCustomSectionLinks,
-    ]);
-
-    this.sectionLinks = this.#initializeSectionLinks(
-      this.defaultMainSectionLinks
-    );
+  #initializeSectionLinks(sectionLinkClasses, { inMoreDrawer } = {}) {
+    return sectionLinkClasses.map((sectionLinkClass) => {
+      return this.#initializeSectionLink(sectionLinkClass, inMoreDrawer);
+    });
  }

-  #initializeSectionLinks(sectionLinkClasses) {
-    return sectionLinkClasses.reduce((links, sectionLinkClass) => {
-      const sectionLink = this.#initializeSectionLink(sectionLinkClass);
-
-      if (sectionLink.shouldDisplay) {
-        links.push(sectionLink);
-      }
-
-      return links;
-    }, []);
-  }
-
-  #initializeSectionLink(sectionLinkClass) {
+  #initializeSectionLink(sectionLinkClass, inMoreDrawer) {
    return new sectionLinkClass({
      topicTrackingState: this.topicTrackingState,
      currentUser: this.currentUser,
      appEvents: this.appEvents,
      router: this.router,
      siteSettings: this.siteSettings,
+      inMoreDrawer,
    });
  }
 }
--- a/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/more-section-link.hbs
@ -1,4 +1,5 @@
 <Sidebar::SectionLink
+  @shouldDisplay={{@sectionLink.shouldDisplay}}
  @linkName={{@sectionLink.name}}
  @route={{@sectionLink.route}}
  @href={{@sectionLink.href}}
--- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs
+++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.hbs
@ -1,68 +1,70 @@
-<div class="sidebar-section-link-wrapper">
-  {{#if @href}}
-    <a href={{@href}} rel="noopener noreferrer" target="_blank" class={{this.classNames}} title={{@title}}>
-      <Sidebar::SectionLinkPrefix
-        @prefixType={{@prefixType}}
-        @prefixValue={{@prefixValue}}
-        @prefixCSSClass={{@prefixCSSClass}}
-        @prefixCSS={{this.prefixCSS}}
-        @prefixBadge={{@prefixBadge}}
-      />
+{{#if this.shouldDisplay}}
+  <div class="sidebar-section-link-wrapper">
+    {{#if @href}}
+      <a href={{@href}} rel="noopener noreferrer" target="_blank" class={{this.classNames}} title={{@title}}>
+        <Sidebar::SectionLinkPrefix
+          @prefixType={{@prefixType}}
+          @prefixValue={{@prefixValue}}
+          @prefixCSSClass={{@prefixCSSClass}}
+          @prefixCSS={{this.prefixCSS}}
+          @prefixBadge={{@prefixBadge}}
+        />

-      <span class="sidebar-section-link-content-text">
-        {{@content}}
-      </span>
-    </a>
-  {{else}}
-    <Sidebar::SectionLinkTo
-      @class={{this.classNames}}
-      @route={{@route}}
-      @query={{@query}}
-      @models={{this.models}}
-      @current-when={{@currentWhen}}
-      @title={{@title}}
-    >
-
-      <Sidebar::SectionLinkPrefix
-        @prefixType={{@prefixType}}
-        @prefixValue={{@prefixValue}}
-        @prefixCSSClass={{@prefixCSSClass}}
-        @prefixCSS={{this.prefixCSS}}
-        @prefixBadge={{@prefixBadge}}
-      />
-
-      <span class="sidebar-section-link-content-text">
-        {{@content}}
-      </span>
-
-      {{#if @badgeText}}
-        <span class="sidebar-section-link-content-badge">
-          {{@badgeText}}
+        <span class="sidebar-section-link-content-text">
+          {{@content}}
        </span>
-      {{/if}}
+      </a>
+    {{else}}
+      <Sidebar::SectionLinkTo
+        @class={{this.classNames}}
+        @route={{@route}}
+        @query={{@query}}
+        @models={{this.models}}
+        @current-when={{@currentWhen}}
+        @title={{@title}}
+      >

-      {{#if @suffixValue}}
-        <span class={{concat-class "sidebar-section-link-suffix" @suffixType @suffixCSSClass}}>
-          {{#if (eq @suffixType "icon")}}
-            {{d-icon @suffixValue}}
-          {{/if}}
+        <Sidebar::SectionLinkPrefix
+          @prefixType={{@prefixType}}
+          @prefixValue={{@prefixValue}}
+          @prefixCSSClass={{@prefixCSSClass}}
+          @prefixCSS={{this.prefixCSS}}
+          @prefixBadge={{@prefixBadge}}
+        />
+
+        <span class="sidebar-section-link-content-text">
+          {{@content}}
        </span>
-      {{/if}}

-      {{#if @hoverValue}}
-        <span class="sidebar-section-link-hover">
-          <button
-            type="button"
-            title={{@hoverTitle}}
-            class="sidebar-section-hover-button"
-            {{on "click" @hoverAction}}
-            >
-            {{#if (eq @hoverType "icon")}}
-              {{d-icon @hoverValue class="hover-icon"}}
+        {{#if @badgeText}}
+          <span class="sidebar-section-link-content-badge">
+            {{@badgeText}}
+          </span>
+        {{/if}}
+
+        {{#if @suffixValue}}
+          <span class={{concat-class "sidebar-section-link-suffix" @suffixType @suffixCSSClass}}>
+            {{#if (eq @suffixType "icon")}}
+              {{d-icon @suffixValue}}
            {{/if}}
-          </button>
-        </span>
-      {{/if}}
-    </Sidebar::SectionLinkTo>
-  {{/if}}
-</div>
+          </span>
+        {{/if}}
+
+        {{#if @hoverValue}}
+          <span class="sidebar-section-link-hover">
+            <button
+              type="button"
+              title={{@hoverTitle}}
+              class="sidebar-section-hover-button"
+              {{on "click" @hoverAction}}
+              >
+              {{#if (eq @hoverType "icon")}}
+                {{d-icon @hoverValue class="hover-icon"}}
+              {{/if}}
+            </button>
+          </span>
+        {{/if}}
+      </Sidebar::SectionLinkTo>
+    {{/if}}
+  </div>
+{{/if}}
--- a/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
+++ b/app/assets/javascripts/discourse/app/components/sidebar/section-link.js
@ -8,6 +8,14 @@ export default class SectionLink extends Component {
    }
  }

+  get shouldDisplay() {
+    if (this.args.shouldDisplay === undefined) {
+      return true;
+    }
+
+    return this.args.shouldDisplay;
+  }
+
  get classNames() {
    let classNames = [
      "sidebar-section-link",
--- a/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js
+++ b/app/assets/javascripts/discourse/app/components/sidebar/user/community-section.js
@ -1,6 +1,5 @@
 import I18n from "I18n";

-import { bind } from "discourse-common/utils/decorators";
 import Composer from "discourse/models/composer";
 import { getOwner } from "discourse-common/lib/get-owner";
 import PermissionType from "discourse/models/permission-type";
@ -31,50 +30,25 @@ export default class SidebarUserCommunitySection extends SidebarCommonCommunityS
        title: I18n.t("sidebar.sections.community.header_action_title"),
      },
    ];
-
-    this.appEvents.on(
-      "user-reviewable-count:changed",
-      this._refreshSectionLinks
-    );
-  }
-
-  willDestroy() {
-    super.willDestroy(...arguments);
-
-    this.appEvents.off(
-      "user-reviewable-count:changed",
-      this._refreshSectionLinks
-    );
-  }
-
-  @bind
-  _refreshSectionLinks() {
-    return this.refreshSectionLinks();
  }

  get defaultMainSectionLinks() {
-    const links = [
+    return [
      EverythingSectionLink,
      TrackedSectionLink,
      MyPostsSectionLink,
      AdminSectionLink,
+      ReviewSectionLink,
    ];
-
-    if (this.currentUser.reviewable_count > 0) {
-      links.push(ReviewSectionLink);
-    }
-
-    return links;
  }

  get defaultMoreSectionLinks() {
-    const links = [GroupsSectionLink, UsersSectionLink, BadgesSectionLink];
-
-    if (this.currentUser.reviewable_count === 0) {
-      links.push(ReviewSectionLink);
-    }
-
-    return links;
+    return [
+      GroupsSectionLink,
+      UsersSectionLink,
+      BadgesSectionLink,
+      ReviewSectionLink,
+    ];
  }

  get defaultMoreSecondarySectionLinks() {
--- a/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js
+++ b/app/assets/javascripts/discourse/app/lib/sidebar/base-community-section-link.js
@ -8,12 +8,14 @@ export default class BaseCommunitySectionLink {
    appEvents,
    router,
    siteSettings,
+    inMoreDrawer,
  } = {}) {
    this.router = router;
    this.topicTrackingState = topicTrackingState;
    this.currentUser = currentUser;
    this.appEvents = appEvents;
    this.siteSettings = siteSettings;
+    this.inMoreDrawer = inMoreDrawer;
  }

  /**
--- a/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js
+++ b/app/assets/javascripts/discourse/app/lib/sidebar/user/community-section/review-section-link.js
@ -1,8 +1,40 @@
 import I18n from "I18n";

+import { tracked } from "@glimmer/tracking";
+
+import { bind } from "discourse-common/utils/decorators";
 import BaseSectionLink from "discourse/lib/sidebar/base-community-section-link";

 export default class ReviewSectionLink extends BaseSectionLink {
+  @tracked canDisplay;
+
+  constructor() {
+    super(...arguments);
+
+    this._refreshCanDisplay();
+    this.appEvents.on("user-reviewable-count:changed", this._refreshCanDisplay);
+  }
+
+  teardown() {
+    this.appEvents.off(
+      "user-reviewable-count:changed",
+      this._refreshCanDisplay
+    );
+  }
+
+  @bind
+  _refreshCanDisplay() {
+    if (!this.currentUser.can_review) {
+      this.canDisplay = false;
+    }
+
+    if (this.inMoreDrawer) {
+      this.canDisplay = this.currentUser.reviewable_count < 1;
+    } else {
+      this.canDisplay = this.currentUser.reviewable_count > 0;
+    }
+  }
+
  get name() {
    return "review";
  }
@ -20,7 +52,7 @@ export default class ReviewSectionLink extends BaseSectionLink {
  }

  get shouldDisplay() {
-    return this.currentUser.can_review;
+    return this.canDisplay;
  }

  get badgeText() {