From a17f1247a8e8e431943a1fadc13e2ef802fbc109 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sun, 20 Sep 2020 00:35:36 -0400 Subject: [PATCH 1/7] Fix `$(...).offset() is undefined` on some scrolls. --- .../core/js/src/forum/components/PostStream.js | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/framework/core/js/src/forum/components/PostStream.js b/framework/core/js/src/forum/components/PostStream.js index afb6a2756..dee970fd9 100644 --- a/framework/core/js/src/forum/components/PostStream.js +++ b/framework/core/js/src/forum/components/PostStream.js @@ -355,8 +355,21 @@ export default class PostStream extends Component { this.updateScrubber(); const index = $item.data('index'); 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; }); From 8640ce83b57cbb4d791d0a3d66f2d7167f02353a Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sun, 20 Sep 2020 00:52:28 -0400 Subject: [PATCH 2/7] Execute oncreate scrubber update after loadPromise has completed This way, we ensure that the initial position (and data) of the scrubber is correct. Otherwise, we get blank dates / incorrect location. --- framework/core/js/src/forum/components/PostStreamScrubber.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/js/src/forum/components/PostStreamScrubber.js b/framework/core/js/src/forum/components/PostStreamScrubber.js index deb4cf2cf..93abaaecd 100644 --- a/framework/core/js/src/forum/components/PostStreamScrubber.js +++ b/framework/core/js/src/forum/components/PostStreamScrubber.js @@ -137,7 +137,7 @@ export default class PostStreamScrubber extends Component { setTimeout(() => this.scrollListener.start()); - this.updateScrubberValues({ animate: true, forceHeightChange: true }); + this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: true, forceHeightChange: true })); } onremove() { From d57a9f100ac11d9867584849eea517dbe15a12d9 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Sun, 20 Sep 2020 01:06:46 -0400 Subject: [PATCH 3/7] Only call updateScrubberValues onupdate when necessary 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 This saves us a LOT of unnecessary calls, and makes scrubber movement smoother. --- framework/core/js/src/forum/components/PostStream.js | 2 ++ .../js/src/forum/components/PostStreamScrubber.js | 5 ++++- .../core/js/src/forum/states/PostStreamState.js | 12 ++++++++++++ 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/framework/core/js/src/forum/components/PostStream.js b/framework/core/js/src/forum/components/PostStream.js index dee970fd9..eef7d6b9c 100644 --- a/framework/core/js/src/forum/components/PostStream.js +++ b/framework/core/js/src/forum/components/PostStream.js @@ -353,6 +353,8 @@ export default class PostStream extends Component { return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => { this.updateScrubber(); + this.stream.forceUpdateScrubber = true; + const index = $item.data('index'); m.redraw.sync(); diff --git a/framework/core/js/src/forum/components/PostStreamScrubber.js b/framework/core/js/src/forum/components/PostStreamScrubber.js index 93abaaecd..ef145809b 100644 --- a/framework/core/js/src/forum/components/PostStreamScrubber.js +++ b/framework/core/js/src/forum/components/PostStreamScrubber.js @@ -90,7 +90,10 @@ export default class PostStreamScrubber extends Component { } onupdate() { - this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: true, forceHeightChange: true })); + if (this.stream.forceUpdateScrubber) { + this.stream.forceUpdateScrubber = false; + this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: true, forceHeightChange: true })); + } } oncreate(vnode) { diff --git a/framework/core/js/src/forum/states/PostStreamState.js b/framework/core/js/src/forum/states/PostStreamState.js index 356bfa18d..355db56be 100644 --- a/framework/core/js/src/forum/states/PostStreamState.js +++ b/framework/core/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); } From 63d0730784b941d624bdc9ab391e7e7c23fdacfa Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 25 Sep 2020 15:43:41 -0400 Subject: [PATCH 4/7] Don't animate the initial Scrubber placement --- framework/core/js/src/forum/components/PostStreamScrubber.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/framework/core/js/src/forum/components/PostStreamScrubber.js b/framework/core/js/src/forum/components/PostStreamScrubber.js index ef145809b..d45474f05 100644 --- a/framework/core/js/src/forum/components/PostStreamScrubber.js +++ b/framework/core/js/src/forum/components/PostStreamScrubber.js @@ -140,7 +140,7 @@ export default class PostStreamScrubber extends Component { setTimeout(() => this.scrollListener.start()); - this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: true, forceHeightChange: true })); + this.stream.loadPromise.then(() => this.updateScrubberValues({ animate: false, forceHeightChange: true })); } onremove() { From 71518112b381c113dc2380d1a688c8c30f03e3f6 Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 25 Sep 2020 15:52:18 -0400 Subject: [PATCH 5/7] Provide location data to scrubber earlier to avoid unnecessary and confusing scrubber animation on page load. --- .../core/js/src/forum/components/PostStream.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/framework/core/js/src/forum/components/PostStream.js b/framework/core/js/src/forum/components/PostStream.js index eef7d6b9c..404913511 100644 --- a/framework/core/js/src/forum/components/PostStream.js +++ b/framework/core/js/src/forum/components/PostStream.js @@ -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,11 +352,16 @@ export default class PostStream extends Component { } } - return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => { - this.updateScrubber(); - this.stream.forceUpdateScrubber = true; + // 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(); + // 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; - const index = $item.data('index'); + return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => { m.redraw.sync(); // After post data has been loaded in, we will attempt to scroll back From 2d7f8130c82b0b69188bd124a8dc24243b55968e Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 25 Sep 2020 15:54:54 -0400 Subject: [PATCH 6/7] DiscussionPage: only set `this.discussion` after the initial set of posts has loaded, this results in a slightly smoother initial load. --- .../core/js/src/forum/components/DiscussionPage.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/framework/core/js/src/forum/components/DiscussionPage.js b/framework/core/js/src/forum/components/DiscussionPage.js index b168a68a0..6fc97119e 100644 --- a/framework/core/js/src/forum/components/DiscussionPage.js +++ b/framework/core/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); + app.current.set('discussion', discussion); + app.current.set('stream', this.stream); + }); } /** From 98c684c97908337993ee64a6d7a1c0e8c4d9af4f Mon Sep 17 00:00:00 2001 From: Alexander Skvortsov Date: Fri, 25 Sep 2020 16:02:39 -0400 Subject: [PATCH 7/7] Fixed noAnimation: previously, the opposite of what was requested happened --- framework/core/js/src/forum/components/PostStream.js | 4 ++-- framework/core/js/src/forum/states/PostStreamState.js | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/framework/core/js/src/forum/components/PostStream.js b/framework/core/js/src/forum/components/PostStream.js index 404913511..df3bfb10c 100644 --- a/framework/core/js/src/forum/components/PostStream.js +++ b/framework/core/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; diff --git a/framework/core/js/src/forum/states/PostStreamState.js b/framework/core/js/src/forum/states/PostStreamState.js index 355db56be..277fd4959 100644 --- a/framework/core/js/src/forum/states/PostStreamState.js +++ b/framework/core/js/src/forum/states/PostStreamState.js @@ -104,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 @@ -128,7 +128,7 @@ class PostStreamState { this.loadPromise = this.loadNearIndex(index); this.targetPost = { index }; - this.noAnimationScroll = noAnimation; + this.animateScroll = !noAnimation; this.index = index; m.redraw();