FEATURE: g,j and g,k to navigate to next and prev topic

FEATURE: g,j and g,k to navigate to next and prev topic

After visiting a topic list (by tag / category / top level) we track the list

Once a list is tracked the combo g j can be used to go to the next topic in the list and g k to go to previous topic.

This allows you to quickly work through subsets of topics without having to navigate back to the top level lists

The shortcut does not work in PM lists yet, or search results, both are under consideration.

diff --git a/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js b/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js
index 2199e6b..7e380bd 100644
--- a/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js
+++ b/app/assets/javascripts/discourse/app/controllers/keyboard-shortcuts-help.js
@@ -69,7 +69,9 @@ export default Controller.extend(ModalFunctionality, {
         bookmarks: buildShortcut("jump_to.bookmarks", { keys1: ["g", "b"] }),
         profile: buildShortcut("jump_to.profile", { keys1: ["g", "p"] }),
         messages: buildShortcut("jump_to.messages", { keys1: ["g", "m"] }),
-        drafts: buildShortcut("jump_to.drafts", { keys1: ["g", "d"] })
+        drafts: buildShortcut("jump_to.drafts", { keys1: ["g", "d"] }),
+        next: buildShortcut("jump_to.next", { keys1: ["g", "j"] }),
+        previous: buildShortcut("jump_to.previous", { keys1: ["g", "k"] })
       },
       navigation: {
         back: buildShortcut("navigation.back", { keys1: ["u"] }),
diff --git a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js
index 50cc5ee..2095c02 100644
--- a/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js
+++ b/app/assets/javascripts/discourse/app/lib/keyboard-shortcuts.js
@@ -5,6 +5,10 @@ import { minimumOffset } from "discourse/lib/offset-calculator";
 import { ajax } from "discourse/lib/ajax";
 import { throttle, schedule } from "@ember/runloop";
 import { INPUT_DELAY } from "discourse-common/config/environment";
+import {
+  nextTopicUrl,
+  previousTopicUrl
+} from "discourse/lib/topic-list-tracker";
 
 const DEFAULT_BINDINGS = {
   "!": { postAction: "showFlags" },
@@ -36,6 +40,8 @@ const DEFAULT_BINDINGS = {
   "g m": { path: "/my/messages" },
   "g d": { path: "/my/activity/drafts" },
   "g s": { handler: "goToFirstSuggestedTopic", anonymous: true },
+  "g j": { handler: "goToNextTopic", anonymous: true },
+  "g k": { handler: "goToPreviousTopic", anonymous: true },
   home: { handler: "goToFirstPost", anonymous: true },
   "command+up": { handler: "goToFirstPost", anonymous: true },
   j: { handler: "selectDown", anonymous: true },
@@ -228,6 +234,22 @@ export default {
     return false;
   },
 
+  goToNextTopic() {
+    nextTopicUrl().then(url => {
+      if (url) {
+        DiscourseURL.routeTo(url);
+      }
+    });
+  },
+
+  goToPreviousTopic() {
+    previousTopicUrl().then(url => {
+      if (url) {
+        DiscourseURL.routeTo(url);
+      }
+    });
+  },
+
   goToFirstSuggestedTopic() {
     const $el = $(".suggested-topics a.raw-topic-link:first");
     if ($el.length) {
diff --git a/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js b/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js
new file mode 100644
index 0000000..c6a5ac3
--- /dev/null
+++ b/app/assets/javascripts/discourse/app/lib/topic-list-tracker.js
@@ -0,0 +1,56 @@
+import { Promise } from "rsvp";
+let model, currentTopicId;
+
+export function setTopicList(incomingModel) {
+  model = incomingModel;
+  currentTopicId = null;
+}
+
+export function nextTopicUrl() {
+  return urlAt(1);
+}
+
+export function previousTopicUrl() {
+  return urlAt(-1);
+}
+
+function urlAt(delta) {
+  if (!model || !model.topics) {
+    return Promise.resolve(null);
+  }
+
+  let index = currentIndex();
+  if (index === -1) {
+    index = 0;
+  } else {
+    index += delta;
+  }
+
+  const topic = model.topics[index];
+
+  if (!topic && index > 0 && model.more_topics_url && model.loadMore) {
+    return model.loadMore().then(() => urlAt(delta));
+  }
+
+  if (topic) {
+    currentTopicId = topic.id;
+    return Promise.resolve(topic.lastUnreadUrl);
+  }
+
+  return Promise.resolve(null);
+}
+
+export function setTopicId(topicId) {
+  currentTopicId = topicId;
+}
+
+function currentIndex() {
+  if (currentTopicId && model && model.topics) {
+    const idx = _.findIndex(model.topics, t => t.id === currentTopicId);
+    if (idx > -1) {
+      return idx;
+    }
+  }
+
+  return -1;
+}
diff --git a/app/assets/javascripts/discourse/app/routes/discovery.js b/app/assets/javascripts/discourse/app/routes/discovery.js
index 38059e2..43f030d 100644
--- a/app/assets/javascripts/discourse/app/routes/discovery.js
+++ b/app/assets/javascripts/discourse/app/routes/discovery.js
@@ -6,6 +6,7 @@ import DiscourseRoute from "discourse/routes/discourse";
 import OpenComposer from "discourse/mixins/open-composer";
 import { scrollTop } from "discourse/mixins/scroll-top";
 import User from "discourse/models/user";
+import { setTopicList } from "discourse/lib/topic-list-tracker";
 
 export default DiscourseRoute.extend(OpenComposer, {
   queryParams: {
@@ -46,6 +47,9 @@ export default DiscourseRoute.extend(OpenComposer, {
     didTransition() {
       this.controllerFor("discovery")._showFooter();
       this.send("loadingComplete");
+
+      const model = this.controllerFor("discovery/topics").get("model");
+      setTopicList(model);
       return false;
     },
 
diff --git a/app/assets/javascripts/discourse/app/routes/tags-show.js b/app/assets/javascripts/discourse/app/routes/tags-show.js
index 74a7ad5..1b26424 100644
--- a/app/assets/javascripts/discourse/app/routes/tags-show.js
+++ b/app/assets/javascripts/discourse/app/routes/tags-show.js
@@ -11,6 +11,7 @@ import PermissionType from "discourse/models/permission-type";
 import Category from "discourse/models/category";
 import FilterModeMixin from "discourse/mixins/filter-mode";
 import { escapeExpression } from "discourse/lib/utilities";
+import { setTopicList } from "discourse/lib/topic-list-tracker";
 
 export default DiscourseRoute.extend(FilterModeMixin, {
   navMode: "latest",
@@ -99,6 +100,9 @@ export default DiscourseRoute.extend(FilterModeMixin, {
           staff: list.topic_list.tags[0].staff
         });
       }
+
+      setTopicList(list);
+
       controller.setProperties({
         list,
         canCreateTopic: list.get("can_create_topic"),
diff --git a/app/assets/javascripts/discourse/app/routes/topic.js b/app/assets/javascripts/discourse/app/routes/topic.js
index 6f7fd41..55c8a74 100644
--- a/app/assets/javascripts/discourse/app/routes/topic.js
+++ b/app/assets/javascripts/discourse/app/routes/topic.js
@@ -4,6 +4,7 @@ import { cancel, later, schedule } from "@ember/runloop";
 import DiscourseRoute from "discourse/routes/discourse";
 import DiscourseURL from "discourse/lib/url";
 import { ID_CONSTRAINT } from "discourse/models/topic";
+import { setTopicId } from "discourse/lib/topic-list-tracker";
 
 const SCROLL_DELAY = 500;
 
@@ -200,7 +201,10 @@ const TopicRoute = DiscourseRoute.extend({
     },
 
     didTransition() {
-      this.controllerFor("topic")._showFooter();
+      const controller = this.controllerFor("topic");
+      controller._showFooter();
+      const topicId = controller.get("model.id");
+      setTopicId(topicId);
       return true;
     },
 
diff --git a/app/assets/javascripts/discourse/app/templates/modal/keyboard-shortcuts-help.hbs b/app/assets/javascripts/discourse/app/templates/modal/keyboard-shortcuts-help.hbs
index 813ab3a..5140803 100644
--- a/app/assets/javascripts/discourse/app/templates/modal/keyboard-shortcuts-help.hbs
+++ b/app/assets/javascripts/discourse/app/templates/modal/keyboard-shortcuts-help.hbs
@@ -15,6 +15,8 @@
           <li>{{html-safe shortcuts.jump_to.messages}}</li>
         {{/if}}
         <li>{{html-safe shortcuts.jump_to.drafts}}</li>
+        <li>{{html-safe shortcuts.jump_to.next}}</li>
+        <li>{{html-safe shortcuts.jump_to.previous}}</li>
       </ul>
     </section>
     <section>
diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml
index ee36917..c881feb 100644
--- a/config/locales/client.en.yml
+++ b/config/locales/client.en.yml
@@ -3185,6 +3185,8 @@ en:
         profile: "%{shortcut} Profile"
         messages: "%{shortcut} Messages"

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

GitHub sha: dc14d156

1 Like

@eviltrout this is missing testing, I would need to mock entire topic lists to test it, let me know if you feel we need an acceptance test, I can work it out.

Also let me know if there is a pattern you prefer, I guess we could use session here, but I think I would like to support this shortcut for search results as well so we may need to massage the data a bit since search results do not have a unified topic list data model.

This commit has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/previous-next-topic/24884/37

If you are putting state in modules you will want to be a good citizen and add a reset() method and call that during Qunit’s test done. Otherwise the state will bleed between tests.

2 Likes

Since every path returns a promise from this method, I’m curious why you didn’t use the return new Promise((resolve, reject) => { ... } API? If you start the method with this you can return resolve(xyz) from the same promise.

1 Like

There should already be topic lists ready if you do an acceptance test. You could do something like:

import { nextTopicUrl, previousTopicUrl } from "topic-list-tracker";

// ... now inside your test, something like:

await visit("/latest");
assert.equal(previousTopicUrl(), null);
assert.equal(nextTopicUrl(), "/t/123");

I would skip the keyboard part because that’s hard to test but the above should be pretty straightforward.

2 Likes

I think this line becomes a bit harder to reason about cause we are switching the promise. I can’t wait till we have we have async so we don’t have to worry about this stuff.

I guess this line would become this, but it is a bit confusing:

return model.loadMore().then(() => { resolve(urlAt(delta)) });

I tried to get the testing going here:

2 Likes

Thanks seems good to me.

1 Like