Toggle fixed header on scroll #1474

jonahtanjz · 2021-02-10T09:56:48Z

What is the purpose of this pull request?

Overview of changes:
Resolves #980.

As discussed, fixed header might take up too much space especially on devices with smaller screen, leaving less space for content. A workaround is to hide the fixed header when scrolling down to give more space to content. Header can still be easily accessed by scrolling up.

Anything you'd like to highlight / discuss:
Should the header hide on both mobile and desktop version when scrolling down? Since this issue affect mainly mobile users, we can continue to fixed the header for desktop version so the header does not need to be hidden there. The current implementation works on both versions.

Testing instructions:
npm run test

Proposed commit message: (wrap lines at 72 characters)
Implement fixed header toggling on scroll

Checklist: ☑️

Updated the documentation for feature additions and enhancements
Added tests for bug fixes or features
Linked all related issues
No blatantly unrelated changes
Pinged someone for a review!

jonahtanjz · 2021-02-10T10:00:40Z

packages/vue-components/src/Overlay.vue

+        document.body.style.overflow = 'hidden';
+      } else {
+        document.body.style.removeProperty('overflow');
+      }


This extra block is needed so that when the overlay is opened, it prevents the header from hiding as a result of scroll chaining scrolling the body.

Can I just clarify what you mean by this?

Screen.Recording.2021-02-11.at.12.27.33.AM.mov

Are you referring to the above where the navbar is hidden partially when you scroll down?

Would it be possible to have a transition effect from no header <-> show header? Without it, I feel that the transition might be a little abrupt. What do you think @jonahtanjz ?

I agree with @raysonkoh on this. Actually, I was thinking if the overlay effect that I mentioned above would be a suitable "transition" effect? I don't see many transition effect options that are suitable for this, to be honest :O

I've dabbled around the transitions and here's something I came up with using max-height css transition. It's not the best solution but I was hoping that it may help with the discussion :-)

Screen.Recording.2021-02-11.at.1.23.58.AM.mov

Thanks for the suggestions @wxwxwxwx9

Can I just clarify what you mean by this?

This is when either the site or page nav is opened and the user scrolls to the end, if the user continues scrolling, it will scroll the main body as well. Can read more about it here: https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior. You can also try it out on the current user guide's site nav: https://markbind-master.netlify.app/userguide/gettingstarted.

I agree with @raysonkoh on this. Actually, I was thinking if the overlay effect that I mentioned above would be a suitable "transition" effect? I don't see many transition effect options that are suitable for this, to be honest :O

Yup agree with you and @raysonkoh on the transition and I was planning to implement it but was kind of stuck on this as the standard css transitions doesn't really work well here. Will have to find alternatives.

I've dabbled around the transitions and here's something I came up with using max-height css transition. It's not the best solution but I was hoping that it may help with the discussion :-)

This actually looks good. Thanks for looking into this 👍. I'm ok with using this. @raysonkoh what do you think about this?

Yup I think it looks good. We can go with this. 👍

This is when either the site or page nav is opened and the user scrolls to the end, if the user continues scrolling, it will scroll the main body as well. Can read more about it here: https://developer.mozilla.org/en-US/docs/Web/CSS/overscroll-behavior. You can also try it out on the current user guide's site nav: https://markbind-master.netlify.app/userguide/gettingstarted.

Ah, I see! Understood. I tried out another solution: place overscroll-behaviour-y: none under .nav-menu. It's working on my side. Can you try if it works for you? I think it's a more succinct solution but I'm not sure if it would interfere with the features you have implemented so far (e.g. siteNav, navBar, and this PR). What do you think? :-)

Yup agree that using this css would be much more succinct solution as I initially wanted to use this as well. However, for some reason, it does not work very well when the nav menus are not scrollable. One example is https://markbind-master.netlify.app/userguide/tweakingthepagestructure, able to scroll the body of page nav when scrolling the ride side of the overlay. This is more obvious on larger screen sizes.

MarkBind.-.User.Guide_.Tweaking.the.Page.Structure.-.Google.Chrome.2021-02-12.11-30-45.mp4

Ahh, I see! Yup, it's not working on my side as well. I think it's because when nav menus are not scrollable (not enough content), overscroll behaviour does not kick in since there's nothing to scroll.

In that case, I think your solution of making the body hidden when overlay is opened makes sense, since we don't want them to be able to scroll the body content when they have overlay opened (since the focus should be on overlay).

For easy reference in the future as to why we add this block, I think we should add some comments to explain the rationale. Maybe something like document.body.style.overflow = 'hidden'; // to prevent scrolling of the body when overlay is overscrolled.

how about just @scroll.stop on the root element of the overlay to prevent the scroll event from bubbling up?

raysonkoh · 2021-02-10T16:02:54Z

Would it be possible to have a transition effect from no header <-> show header? Without it, I feel that the transition might be a little abrupt. What do you think @jonahtanjz ?

raysonkoh · 2021-02-10T16:11:29Z

Should the header hide on both mobile and desktop version when scrolling down? Since this issue affect mainly mobile users, we can continue to fixed the header for desktop version so the header does not need to be hidden there. The current implementation works on both versions.

I feel that we can continue to fix the header for desktop version since they already have enough screen real estate and the header wouldn't occupy much of the screen.

wxwxwxwx9 · 2021-02-10T16:51:22Z

@jonahtanjz There's currently a weird behavior when I overscroll the page.

Screen.Recording.2021-02-11.at.12.50.16.AM.mov

wxwxwxwx9 · 2021-02-10T17:27:17Z

packages/core-web/src/index.js

  addResizeHeaderListener();
+  setTimeout(() => window.addEventListener('scroll', toggleHeaderOnScroll), 500);


Can I check why do we need this setTimeout here? :O

This is for rare situations where the size of the navbar can change when the page is refreshed. For example, when a <box dismissable> is placed in the header. This will cause the navbar to change size while rendering resulting in an unwanted scroll event to fire before the page is fully loaded. Although not the best way to counter this, a delay here would prevent the unwanted scroll event to be picked up. I believe the timeout would not be necessary anymore after #1406 is implemented.

Landing.Page.-.Google.Chrome.2021-02-11.14-10-38.mp4

Thanks for the explanation! I'm not sure if setTimeout would be appropriate in the case where the user has slow loading speed (e.g. page takes longer than the stipulated 500ms to load). I think the solution here won't work in this scenario. However, I'm also unsure if there is a better way to solve this issue :-/

I tried looking through the list of event hooks that we can possibly use to detect when the page is fully-rendered but I don't think I found any.

https://developer.mozilla.org/en-US/docs/Web/Events

I also tried a few other hacky ways to get a solution to work but no luck so far.

But a surefire way to solve this problem at some expense can be window.history.scrollRestoration = 'manual';, where we always start off at the top whenever a page is reloaded, instead of where the user was at previously. I'm not sure if this is the way we want to go forward but I think it's a potential solution. What do you think?

Thanks for investigating this @wxwxwxwx9 👍. Since this issue is only apparent on desktop version of the navbar, if we follow @raysonkoh suggestion of only hiding the header on mobile, then the unwanted scroll event will not be captured on desktop version. I've tested the mobile version and it works fine as the box is contained within the dropdown so a reload will not trigger a scroll.

Alright! That would work as well. :-)

jonahtanjz · 2021-02-11T05:55:27Z

Would it be possible to have a transition effect from no header <-> show header? Without it, I feel that the transition might be a little abrupt. What do you think @jonahtanjz ?

Thanks for the suggestion @raysonkoh 👍. Agree with you on the transition. @wxwxwxwx9 came up with a possible transition effect on the navbar discussed above. What do you think about that?

jonahtanjz · 2021-02-11T06:14:00Z

@jonahtanjz There's currently a weird behavior when I overscroll the page.

Thanks for the catch! @wxwxwxwx9 👍

jonahtanjz · 2021-02-12T04:18:45Z

@raysonkoh and @wxwxwxwx9 I've updated the PR to include transitions and addressed some of the bugs/issues.

wxwxwxwx9 · 2021-02-12T09:08:35Z

packages/core-web/src/index.js

+  const toggleHeaderOnScroll = () => {
+    if (window.innerWidth > 767) { return; }
+    const currentOffset = window.pageYOffset;
+    if ((window.innerHeight + currentOffset) >= document.body.offsetHeight) { return; }


Hmm I think we can add some comment here as to why this is needed. I wasn't quite sure what this was for until I tested it out. And maybe we can assign it to a variable as well, maybe something like isEndOfPage.

Likewise for the above window.innerWidth > 767 as well :-), something more descriptive to help readers understand quicker in the future.

Yup I agree with @wxwxwxwx9 .

Other than that everything LGTM! Thanks for making the changes @jonahtanjz !

Thanks for the suggestion @wxwxwxwx9 and @raysonkoh. Have added the comments :)

ang-zeyu

Thanks! @jonahtanjz

This mostly looks good. 🙂
Couple of suggestions / issues:

ang-zeyu · 2021-02-13T03:28:45Z

packages/vue-components/src/Overlay.vue

+        document.body.style.overflow = 'hidden';
+      } else {
+        document.body.style.removeProperty('overflow');
+      }


how about just @scroll.stop on the root element of the overlay to prevent the scroll event from bubbling up?

ang-zeyu · 2021-02-13T03:33:13Z

packages/core-web/src/styles/markbind.css

@@ -118,11 +118,19 @@ code.hljs.inline {
 /* Header */

 header[fixed] {
+    max-height: 100%;
+    overflow: hidden;


will need to be dynamically adjusted (visible once it reaches max-height 100%, hidden otherwise)

searchbar:

I've managed to fixed this by moving the overflow: hidden to a media query for mobile. Seems to be working for both mobile and desktop sites. Is this approach ok instead of looking at the max-height?

hmm I guessing it may not work well if the user uses a popover, etc. in the header

also there's the upper topnav of items in #980 and how that might work with dropdowns (might overflow similarly)

Alright got it 👍. Updated overflow to be dynamic such that it will only be visible if max-height is 100%.

ang-zeyu · 2021-02-13T03:47:07Z

packages/core-web/src/index.js

+    }
+    lastOffset = currentOffset;
+  };
+
  addResizeHeaderListener();


could we prevent this from running when caused by the changes in max-height?

so we don't observe the secondary movements of the content when hiding / showing the navbar

Alright updated :)

jonahtanjz · 2021-02-13T06:05:37Z

how about just @scroll.stop on the root element of the overlay to prevent the scroll event from bubbling up?

I tried adding this but doesn't seem to be working. I also tried using e.stopPropagation() but it's not working as well 😕

ang-zeyu · 2021-02-13T07:53:23Z

how about just @scroll.stop on the root element of the overlay to prevent the scroll event from bubbling up?

I tried adding this but doesn't seem to be working. I also tried using e.stopPropagation() but it's not working as well 😕

oops, looks like I misunderstood the issue here (scrolling, not the event is bubbling up), let's leave this as is then!

ang-zeyu · 2021-02-14T07:25:21Z

packages/core-web/src/index.js

+      if (headerMaxHeight !== '100%') {
+        headerSelector.css('overflow', 'hidden');
+        // re-check after 0.6s to account for transition time
+        setTimeout(toggleHeaderOverflow, 600);


Generally not a good idea to use setTimeout unless a delay / timer is explicitly needed, as the timing of events (here being the transition finishing and resize handler) is not guaranteed. (especially across browsers / devices)

You could take a look at the https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/transitionend_event (and run) here instead to detect whether it is transitioning / finished transitioning

Thanks for the suggestion @ang-zeyu :) Was not aware of this transition event. This makes it much better. Have removed the setTimeout and replaced it with transitionend listener.

ang-zeyu

Thanks! Last few nits:

ang-zeyu · 2021-02-14T09:12:16Z

packages/core-web/src/index.js

@@ -52,38 +52,75 @@ function detectAndApplyFixedHeaderStyles() {
      }`);
  insertCss(`.nav-menu-open { max-height: calc(100% - ${headerHeight}px); }`);

+  const resizeHeader = () => {


Suggested change

const resizeHeader = () => {

const adjustHeaderClasses = () => {

since the header itself isn't resized =P

ang-zeyu · 2021-02-14T09:12:28Z

packages/core-web/src/index.js

+      resizeHeader();
+    }
+  };
+
  const addResizeHeaderListener = () => {


should we remove the wrapping method? seems a little verbose now that you've extracted a huge chunk out

ang-zeyu · 2021-02-14T09:15:56Z

packages/core-web/src/index.js

-          }
-        }
+      const headerMaxHeight = headerSelector.css('max-height');
+      // set overflow to hidden if max-height is not 100%


let's explain the why here instead

ang-zeyu · 2021-02-14T09:18:01Z

packages/core-web/src/index.js

+
+  const toggleHeaderOverflow = () => {
+    const headerMaxHeight = headerSelector.css('max-height');
+    if (headerMaxHeight === '100%') {


a small why (we need to reset overflow when the header shows again) comment here would help too

jonahtanjz · 2021-02-14T09:56:57Z

@ang-zeyu Updated the PR :)

ang-zeyu

Lgtm 👍

jonahtanjz commented Feb 10, 2021

View reviewed changes

wxwxwxwx9 reviewed Feb 10, 2021

View reviewed changes

jonahtanjz changed the title ~~[WIP] Toggle fixed header on scroll~~ Toggle fixed header on scroll Feb 12, 2021

jonahtanjz added 2 commits February 12, 2021 11:53

Hide header on scroll

410985e

Add transition effect

f049d2c

jonahtanjz force-pushed the hide-navbar-onscroll branch from 48d9e06 to f049d2c Compare February 12, 2021 03:54

wxwxwxwx9 reviewed Feb 12, 2021

View reviewed changes

Add comments to code

587be39

ang-zeyu reviewed Feb 13, 2021

View reviewed changes

Remove overflow when max-height is 100%

dece3cc

ang-zeyu reviewed Feb 14, 2021

View reviewed changes

Add event listener for tranistion end

8fbe4d0

ang-zeyu reviewed Feb 14, 2021

View reviewed changes

Update after review

c7c2496

ang-zeyu approved these changes Feb 14, 2021

View reviewed changes

ang-zeyu added this to the v3.0 milestone Feb 14, 2021

ang-zeyu merged commit adc6f82 into MarkBind:master Feb 14, 2021

		addResizeHeaderListener();
		setTimeout(() => window.addEventListener('scroll', toggleHeaderOnScroll), 500);

	const resizeHeader = () => {
	const adjustHeaderClasses = () => {

Toggle fixed header on scroll #1474

Toggle fixed header on scroll #1474

Conversation

jonahtanjz commented Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

wxwxwxwx9 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxwxwxwx9 Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

jonahtanjz Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raysonkoh commented Feb 10, 2021

raysonkoh commented Feb 10, 2021

wxwxwxwx9 commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wxwxwxwx9 Feb 11, 2021 • edited Loading

Choose a reason for hiding this comment

jonahtanjz Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 11, 2021

jonahtanjz commented Feb 11, 2021 • edited Loading

jonahtanjz commented Feb 12, 2021

wxwxwxwx9 Feb 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 13, 2021

ang-zeyu commented Feb 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ang-zeyu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 14, 2021

ang-zeyu left a comment

Choose a reason for hiding this comment

jonahtanjz commented Feb 10, 2021 •

edited

Loading

wxwxwxwx9 Feb 10, 2021 •

edited

Loading

wxwxwxwx9 Feb 10, 2021 •

edited

Loading

jonahtanjz Feb 11, 2021 •

edited

Loading

wxwxwxwx9 Feb 11, 2021 •

edited

Loading

jonahtanjz Feb 12, 2021 •

edited

Loading

jonahtanjz commented Feb 11, 2021 •

edited

Loading

wxwxwxwx9 Feb 12, 2021 •

edited

Loading