Merge pull request #2299 from flarum/as/poststream_improvements

[Frontend Rewrite] PostStream Improvements

js/src/forum/components/DiscussionPage.js

@@ -160,10 +160,8 @@ export default class DiscussionPage extends Page {
    * @param {Discussion} discussion
    */
   show(discussion) {
-    this.discussion = discussion;
-
     app.history.push('discussion', discussion.title());
-    app.setTitle(this.discussion.title());
+    app.setTitle(discussion.title());
     app.setTitleCount(0);
 
     // When the API responds with a discussion, it will also include a number of
@@ -194,10 +192,12 @@ export default class DiscussionPage extends Page {
     // posts we want to display. Tell the stream to scroll down and highlight
     // the specific post that was routed to.
     this.stream = new PostStreamState(discussion, includedPosts);
-    this.stream.goToNumber(m.route.param('near') || (includedPosts[0] && includedPosts[0].number()), true);
+    this.stream.goToNumber(m.route.param('near') || (includedPosts[0] && includedPosts[0].number()), true).then(() => {
+      this.discussion = discussion;
 
       app.current.set('discussion', discussion);
       app.current.set('stream', this.stream);
+    });
   }
 
   /**

js/src/forum/components/PostStream.js

@@ -132,10 +132,10 @@ export default class PostStream extends Component {
     }
 
     if ('number' in newTarget) {
-      this.scrollToNumber(newTarget.number, this.stream.noAnimationScroll);
+      this.scrollToNumber(newTarget.number, this.stream.animateScroll);
     } else if ('index' in newTarget) {
       const backwards = newTarget.index === this.stream.count() - 1;
-      this.scrollToIndex(newTarget.index, this.stream.noAnimationScroll, backwards);
+      this.scrollToIndex(newTarget.index, this.stream.animateScroll, backwards);
     }
 
     this.prevTarget = newTarget;
@@ -330,6 +330,7 @@ export default class PostStream extends Component {
    */
   scrollToItem($item, animate, force, bottom) {
     const $container = $('html, body').stop(true);
+    const index = $item.data('index');
 
     if ($item.length) {
       const itemTop = $item.offset().top - this.getMarginTop();
@@ -351,12 +352,32 @@ export default class PostStream extends Component {
       }
     }
 
-    return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => {
+    // Even before scrolling, we want to provide location information
+    // to the scrubber as soon as possible. This way, we avoid an unnecessary
+    // and confusing animation.
     this.updateScrubber();
-      const index = $item.data('index');
+    // We manually set the index because we want to display the index of the
+    // exact post we've scrolled to, not just that of the first post on the page.
+    this.stream.index = index;
+    this.stream.forceUpdateScrubber = true;
+
+    return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => {
       m.redraw.sync();
-      const scroll = index == 0 ? 0 : $(`.PostStream-item[data-index=${$item.data('index')}]`).offset().top - this.getMarginTop();
-      $(window).scrollTop(scroll);
+
+      // After post data has been loaded in, we will attempt to scroll back
+      // to the top of the requested post (or to the top of the page if the
+      // first post was requested). In some cases, we may have scrolled to
+      // the end of the available post range, in which case, the next range
+      // of posts will be loaded in. However, in those cases, the post we
+      // requested won't exist, so scrolling to it would cause an error.
+      // Accordingly, we start by checking that it's offset is defined.
+      const offset = $(`.PostStream-item[data-index=${index}]`).offset();
+      if (index === 0) {
+        $(window).scrollTop(0);
+      } else if (offset) {
+        $(window).scrollTop($(`.PostStream-item[data-index=${index}]`).offset().top - this.getMarginTop());
+      }
+
       this.calculatePosition();
       this.stream.paused = false;
     });

js/src/forum/components/PostStreamScrubber.js

@@ -90,8 +90,11 @@ export default class PostStreamScrubber extends Component {
   }
 
   onupdate() {
+    if (this.stream.forceUpdateScrubber) {
+      this.stream.forceUpdateScrubber = false;
       this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: true, forceHeightChange: true }));
     }
+  }
 
   oncreate(vnode) {
     super.oncreate(vnode);
@@ -137,7 +140,7 @@ export default class PostStreamScrubber extends Component {
 
     setTimeout(() => this.scrollListener.start());
 
-    this.updateScrubberValues({ animate: true, forceHeightChange: true });
+    this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: false, forceHeightChange: true }));
   }
 
   onremove() {

js/src/forum/states/PostStreamState.js

@@ -37,6 +37,18 @@ class PostStreamState {
      */
     this.description = '';
 
+    /**
+     * When the page is scrolled, goToIndex is called, or the page is loaded,
+     * various listeners result in the scrubber being updated with a new
+     * position and values. However, if goToNumber is called, the scrubber
+     * will not be updated. Accordingly, we add logic to the scrubber's
+     * onupdate to update itself, but only when needed, as indicated by this
+     * property.
+     *
+     * @type {Boolean}
+     */
+    this.forceUpdateScrubber = false;
+
     this.show(includedPosts);
   }
 
@@ -92,7 +104,7 @@ class PostStreamState {
     this.loadPromise = this.loadNearNumber(number);
 
     this.targetPost = { number };
-    this.noAnimationScroll = noAnimation;
+    this.animateScroll = !noAnimation;
     this.number = number;
 
     // In this case, the redraw is only called after the response has been loaded
@@ -116,7 +128,7 @@ class PostStreamState {
     this.loadPromise = this.loadNearIndex(index);
 
     this.targetPost = { index };
-    this.noAnimationScroll = noAnimation;
+    this.animateScroll = !noAnimation;
     this.index = index;
 
     m.redraw();