REFACTOR: scroll-based mobile header switch

REFACTOR: scroll-based mobile header switch

This refactor addresses the following issues:

1- Moves all relevant logic to the discourse-topic component (matches desktop) 2- Fixes the flicker issue discussed here 3- Fixes a rare occurring issue where tags wrap to a third line if a topic has long category names and lots of tags 4- Fixes header icon jitter on iOS 5- Fixes an issue where sliding out user / hamburger menus on Android leaves the user in a mid-state with half a title and the header panel visible - swiping will now open the menus but have no effect on the header. 6- adds min-width to the small-logo to act as placeholder so that the title doesn’t shift if the logo takes a while to load.

Other than that, everything should look and act the same.

diff --git a/app/assets/javascripts/discourse/components/discourse-topic.js.es6 b/app/assets/javascripts/discourse/components/discourse-topic.js.es6
index 4a112c7..fd4ce8f 100644
--- a/app/assets/javascripts/discourse/components/discourse-topic.js.es6
+++ b/app/assets/javascripts/discourse/components/discourse-topic.js.es6
@@ -5,6 +5,10 @@ import Scrolling from "discourse/mixins/scrolling";
 import { selectedText } from "discourse/lib/utilities";
 import { observes } from "ember-addons/ember-computed-decorators";
 
+const MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE = 300;
+// Small buffer so that very tiny scrolls don't trigger mobile header switch
+const MOBILE_SCROLL_TOLERANCE = 5;
+
 function highlight(postNumber) {
   const $contents = $(`#post_${postNumber} .topic-body`);
 
@@ -12,9 +16,6 @@ function highlight(postNumber) {
   $contents.on("animationend", () => $contents.removeClass("highlighted"));
 }
 
-// used to determine scroll direction on mobile
-let lastScroll, scrollDirection, delta;
-
 export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
   userFilters: Ember.computed.alias("topic.userFilters"),
   classNameBindings: [
@@ -34,6 +35,9 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
 
   _lastShowTopic: null,
 
+  mobileScrollDirection: null,
+  _mobileLastScroll: null,
+
   @observes("enteredAt")
   _enteredTopic() {
     // Ember is supposed to only call observers when values change but something
@@ -97,23 +101,6 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
         this.appEvents.trigger("header:hide-topic");
       }
     });
-    // setup mobile scroll logo
-    if (this.site.mobileView) {
-      this.appEvents.on("topic:scrolled", offset =>
-        this.mobileScrollGaurd(offset)
-      );
-      // used to animate header contents on scroll
-      this.appEvents.on("header:show-topic", () => {
-        $("header.d-header")
-          .removeClass("scroll-up")
-          .addClass("scroll-down");
-      });
-      this.appEvents.on("header:hide-topic", () => {
-        $("header.d-header")
-          .removeClass("scroll-down")
-          .addClass("scroll-up");
-      });
-    }
   },
 
   willDestroyElement() {
@@ -129,11 +116,7 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
     // this happens after route exit, stuff could have trickled in
     this.appEvents.trigger("header:hide-topic");
     this.appEvents.off("post:highlight");
-    // mobile scroll logo clean up.
-    if (this.site.mobileView) {
-      this.appEvents.off("topic:scrolled");
-      $("header.d-header").removeClass("scroll-down scroll-up");
-    }
+    this.appEvents.off("header:update-topic");
   },
 
   @observes("Discourse.hasFocus")
@@ -148,17 +131,13 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
   },
 
   showTopicInHeader(topic, offset) {
-    // conditions for showing topic title in the header for mobile
-    if (
-      this.site.mobileView &&
-      scrollDirection !== "up" &&
-      offset > this.dockAt
-    ) {
-      return true;
-      // condition for desktops
-    } else {
-      return offset > this.dockAt;
-    }
+    // On mobile, we show the header topic if the user has scrolled past the topic
+    // title and the current scroll direction is down
+    // On desktop the user only needs to scroll past the topic title.
+    return (
+      offset > this.dockAt &&
+      (!this.site.mobileView || this.mobileScrollDirection === "down")
+    );
   },
   // The user has scrolled the window, or it is finished rendering and ready for processing.
   scrolled() {
@@ -193,25 +172,61 @@ export default Ember.Component.extend(AddArchetypeClass, Scrolling, {
       }
     }
 
+    // Since the user has scrolled, we need to check the scroll direction on mobile.
+    // We use throttle instead of debounce because we want the switch to occur
+    // at the start of the scroll. This feels a lot more snappy compared to waiting
+    // for the scroll to end if we debounce.
+    if (this.site.mobileView && this.hasScrolled) {
+      Ember.run.throttle(
+        this,
+        this._mobileScrollDirectionCheck,
+        offset,
+        MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE
+      );
+    }
+
     // Trigger a scrolled event
     this.appEvents.trigger("topic:scrolled", offset);
   },
 
-  // determines scroll direction, triggers header topic info on mobile
-  // and ensures that the switch happens only once per scroll direction change
-  mobileScrollGaurd(offset) {
-    // user hasn't scrolled past topic title.
-    if (offset < this.dockAt) return;
-
-    delta = offset - lastScroll;
-    // 3px buffer so that the switch doesn't happen with tiny scrolls
-    if (delta > 3 && scrollDirection !== "down") {
-      scrollDirection = "down";
-      this.appEvents.trigger("header:show-topic", this.topic);
-    } else if (delta < -3 && scrollDirection !== "up") {
-      scrollDirection = "up";
-      this.appEvents.trigger("header:hide-topic");
+  _mobileScrollDirectionCheck(offset) {
+    // Difference between this scroll and the one before it.
+    const delta = Math.floor(offset - this._mobileLastScroll);
+
+    // This is a tiny scroll, so we ignore it.
+    if (delta <= MOBILE_SCROLL_TOLERANCE && delta >= -MOBILE_SCROLL_TOLERANCE)
+      return;
+
+    const prevDirection = this.mobileScrollDirection;
+    const currDirection = delta > 0 ? "down" : "up";
+
+    if (currDirection !== prevDirection) {
+      this.set("mobileScrollDirection", currDirection);
     }
-    lastScroll = offset;
+
+    // We store this to compare against it the next time the user scrolls
+    this._mobileLastScroll = Math.floor(offset);
+
+    // If the user reaches the very bottom of the topic, we want to reset the
+    // scroll direction in order for the header to switch back.
+    const distanceToTopicBottom = Math.floor(
+      $("body").height() - offset - $(window).height()
+    );
+
+    // Not at the bottom yet
+    if (distanceToTopicBottom > 0) return;
+
+    // We're at the bottom now, so we reset the direction.
+    this.set("mobileScrollDirection", null);
+  },
+
+  // We observe the scroll direction on mobile and if it's down, we show the topic
+  // in the header, otherwise, we hide it.
+  @observes("mobileScrollDirection")
+  toggleMobileHeaderTopic() {
+    return this.appEvents.trigger(
+      "header:update-topic",
+      this.mobileScrollDirection === "down" ? this.get("topic") : null
+    );
   }
 });
diff --git a/app/assets/javascripts/discourse/components/topic-progress.js.es6 b/app/assets/javascripts/discourse/components/topic-progress.js.es6
index 3990124..ff1ffaa 100644
--- a/app/assets/javascripts/discourse/components/topic-progress.js.es6
+++ b/app/assets/javascripts/discourse/components/topic-progress.js.es6
@@ -1,4 +1,3 @@
-import { getOwner } from "discourse-common/lib/get-owner";
 import {
   default as computed,
   observes
@@ -155,16 +154,17 @@ export default Ember.Component.extend({
     const $wrapper = this.$();
     if (!$wrapper || $wrapper.length === 0) return;
 
-    const $html = $("html"),
-      offset = window.pageYOffset || $html.scrollTop(),
-      progressHeight = this.site.mobileView ? 0 : $("#topic-progress").height(),
-      maximumOffset = $("#topic-bottom").offset().top + progressHeight,
-      windowHeight = $(window).height(),
-      bodyHeight = $("body").height(),
-      composerHeight = $("#reply-control").height() || 0,
-      isDocked = offset >= maximumOffset - windowHeight + composerHeight,
-      bottom = bodyHeight - maximumOffset,
-      wrapperDir = $html.hasClass("rtl") ? "left" : "right";
+    const $html = $("html");
+    const offset = window.pageYOffset || $html.scrollTop();
+    const progressHeight = this.site.mobileView
+      ? 0
+      : $("#topic-progress").height();
+    const maximumOffset = $("#topic-bottom").offset().top + progressHeight;
+    const windowHeight = $(window).height();

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

GitHub sha: 6d7c0c8f

2 Likes

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

Not 100% sure if this commit is the cause, but I’m seeing random parts of the header appearing down the right hand side of the page. It seems worse when I have an unread notification, and even worse on a self-hosted forum with additional buttons in the header.

Is there a reason we can’t display:none; the hidden header, rather than pushing it off screen?

2 Likes

The reason I’m avoiding display: none is because the hamburger menu and the user menu are children of .panel

If we set it to display: none the menus will inherit that and it’s not possible to have visible children under a hidden parent. So, if the user swipes right or left on android to show these menus, they would get stuck.

Looking back at this again, I think it might be much better to target .d-header-icons and .header-buttons instead of .panel - I think then we’d be able to use display: none without issues.

I’ll test this and send a fix.

3 Likes

I added the fix in #7257

2 Likes