FIX: Add bookmark-list component (#10451)

FIX: Add bookmark-list component (#10451)

Moving the bookmark list into its own component to solve click binding issues for external links, because controllers are not the place for DOM manipulation!

diff --git a/app/assets/javascripts/discourse/app/components/bookmark-list.js b/app/assets/javascripts/discourse/app/components/bookmark-list.js
new file mode 100644
index 0000000..1a5917a
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/components/bookmark-list.js
@@ -0,0 +1,60 @@
+import Component from "@ember/component";
+import I18n from "I18n";
+import { action } from "@ember/object";
+import showModal from "discourse/lib/show-modal";
+import {
+  shouldOpenInNewTab,
+  openLinkInNewTab
+} from "discourse/lib/click-track";
+
+export default Component.extend({
+  classNames: ["bookmark-list-wrapper"],
+
+  @action
+  removeBookmark(bookmark) {
+    const deleteBookmark = () => {
+      return bookmark
+        .destroy()
+        .then(() => this._removeBookmarkFromList(bookmark));
+    };
+    if (!bookmark.reminder_at) {
+      return deleteBookmark();
+    }
+    bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => {
+      if (result) {
+        return deleteBookmark();
+      }
+    });
+  },
+
+  @action
+  screenExcerptForExternalLink(event) {
+    if (event.target && event.target.tagName === "A") {
+      let link = event.target;
+      if (shouldOpenInNewTab(link.href)) {
+        openLinkInNewTab(link);
+      }
+    }
+  },
+
+  @action
+  editBookmark(bookmark) {
+    let controller = showModal("bookmark", {
+      model: {
+        postId: bookmark.post_id,
+        id: bookmark.id,
+        reminderAt: bookmark.reminder_at,
+        name: bookmark.name
+      },
+      title: "post.bookmarks.edit",
+      modalClass: "bookmark-with-reminder"
+    });
+    controller.setProperties({
+      afterSave: () => this.reload()
+    });
+  },
+
+  _removeBookmarkFromList(bookmark) {
+    this.content.removeObject(bookmark);
+  }
+});
diff --git a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
index 577f148..c566b62 100644
--- a/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
+++ b/app/assets/javascripts/discourse/app/controllers/user-activity-bookmarks.js
@@ -1,16 +1,10 @@
 import I18n from "I18n";
-import { schedule } from "@ember/runloop";
 import Controller from "@ember/controller";
-import showModal from "discourse/lib/show-modal";
 import { Promise } from "rsvp";
 import { inject } from "@ember/controller";
 import { action } from "@ember/object";
 import discourseComputed from "discourse-common/utils/decorators";
 import Bookmark from "discourse/models/bookmark";
-import {
-  shouldOpenInNewTab,
-  openLinkInNewTab
-} from "discourse/lib/click-track";
 
 export default Controller.extend({
   application: inject(),
@@ -24,11 +18,6 @@ export default Controller.extend({
 
   queryParams: ["q"],
 
-  init() {
-    this._super(...arguments);
-    this._boundClick = false;
-  },
-
   loadItems() {
     this.setProperties({
       content: [],
@@ -40,26 +29,6 @@ export default Controller.extend({
       this.set("searchTerm", this.q);
     }
 
-    if (!this._boundClick) {
-      schedule("afterRender", () => {
-        // TODO(martin): This should be pulled out into a bookmark-list component,
-        // the controller is not the best place for this.
-        let wrapper = document.querySelector(".bookmark-list-wrapper");
-        if (!wrapper) {
-          return;
-        }
-        wrapper.addEventListener("click", function(e) {
-          if (e.target && e.target.tagName === "A") {
-            let link = e.target;
-            if (shouldOpenInNewTab(link.href)) {
-              openLinkInNewTab(link);
-            }
-          }
-        });
-        this._boundClick = true;
-      });
-    }
-
     return this.model
       .loadItems({ q: this.searchTerm })
       .then(response => this._processLoadResponse(response))
@@ -77,10 +46,6 @@ export default Controller.extend({
     return loaded && contentLength === 0 && noResultsHelp;
   },
 
-  _removeBookmarkFromList(bookmark) {
-    this.content.removeObject(bookmark);
-  },
-
   @action
   search() {
     this.set("q", this.searchTerm);
@@ -88,37 +53,8 @@ export default Controller.extend({
   },
 
   @action
-  removeBookmark(bookmark) {
-    const deleteBookmark = () => {
-      return bookmark
-        .destroy()
-        .then(() => this._removeBookmarkFromList(bookmark));
-    };
-    if (!bookmark.reminder_at) {
-      return deleteBookmark();
-    }
-    bootbox.confirm(I18n.t("bookmarks.confirm_delete"), result => {
-      if (result) {
-        return deleteBookmark();
-      }
-    });
-  },
-
-  @action
-  editBookmark(bookmark) {
-    let controller = showModal("bookmark", {
-      model: {
-        postId: bookmark.post_id,
-        id: bookmark.id,
-        reminderAt: bookmark.reminder_at,
-        name: bookmark.name
-      },
-      title: "post.bookmarks.edit",
-      modalClass: "bookmark-with-reminder"
-    });
-    controller.setProperties({
-      afterSave: () => this.loadItems()
-    });
+  reload() {
+    this.loadItems();
   },
 
   @action
diff --git a/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs
new file mode 100644
index 0000000..6d49d71
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/templates/components/bookmark-list.hbs
@@ -0,0 +1,80 @@
+{{#conditional-loading-spinner condition=loading}}
+  {{#load-more selector=".bookmark-list .bookmark-list-item" action=loadMore}}
+    <table class="topic-list bookmark-list">
+      <thead>
+        {{#if site.mobileView}}
+          <th>&nbsp;</th>
+          <th>{{i18n "topic.title"}}</th>
+          <th>&nbsp;</th>
+        {{else}}
+          <th>{{i18n "topic.title"}}</th>
+          <th>&nbsp;</th>
+          <th class="post-metadata">{{i18n "post.bookmarks.updated"}}</th>
+          <th class="post-metadata">{{i18n "activity"}}</th>
+          <th>&nbsp;</th>
+        {{/if}}
+      </thead>
+      <tbody>
+        {{#each content as |bookmark|}}
+          <tr class="topic-list-item bookmark-list-item">
+            {{#if site.mobileView}}
+              <td>
+                {{#if bookmark.post_user_avatar_template}}
+                  <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>
+                    {{avatar bookmark.postUser avatarTemplatePath="avatar_template" usernamePath="username" namePath="name" imageSize="small"}}
+                  </a>
+                {{/if}}
+              </td>
+            {{/if}}
+            <td class="main-link">
+              <span class="link-top-line">
+                <div class="bookmark-metadata">
+                  {{#if bookmark.name}}
+                    <span class="bookmark-metadata-item">
+                      {{d-icon "info-circle"}}{{bookmark.name}}
+                    </span>
+                  {{/if}}
+                  {{#if bookmark.reminder_at}}
+                    <span class="bookmark-metadata-item">
+                      {{d-icon "far-clock"}}{{bookmark.formattedReminder}}
+                    </span>
+                  {{/if}}
+                </div>
+
+                {{topic-status topic=bookmark}}
+                {{topic-link bookmark}}
+              </span>
+              {{#if bookmark.excerpt}}
+                {{!-- template-lint-disable  --}}
+                <p class="post-excerpt" {{on "click" this.screenExcerptForExternalLink}}>{{html-safe bookmark.excerpt}}</p>
+              {{/if}}
+              <div class="link-bottom-line">
+                {{category-link bookmark.category}}
+                {{discourse-tags bookmark mode="list" tagsForUser=tagsForUser}}
+              </div>
+            </td>
+            {{#unless site.mobileView}}
+              <td>
+                {{#if bookmark.post_user_avatar_template}}
+                  <a href={{bookmark.postUser.path}} data-user-card={{bookmark.post_user_username}}>

[... diff too long, it was truncated ...]

GitHub sha: bb74ccf2

1 Like

This commit appears in #10451 which was merged by martin.

This function can return a promise or not, depending on some logic. This means that any other code that calls this must check the return value if they want to apply then or catch logic, in case it’s not a promise.

I highly recommend always returning a promise from this method regardless of what logic happens.

This could be set('afterSave', () => ...), properties is only recommended when updating multiple properties at once.

Fixes in DEV: Review fixes for bookmark-list by martin-brennan · Pull Request #10642 · discourse/discourse · GitHub