feat: hides/shows header on scroll #592

bungandy · 2021-08-08T18:45:58Z

Hides/shows header on scrolling a page

Why?

"When implemented wrong, sticky navigation elements can serve as a distraction from the actual content.”
– Aaron Andre Chichioco

Objective

If user is scrolling down, header area turn into invisible format to increase the area, so user can have more 'room' or reducing distraction elements. And when user is scrolling up, the header / burger menu become visible.

I know the header area quite small as it has 64px height, but fixed header has the downside of taking up a significant portion of the smaller mobile screen. The same pattern can be also seen in mobile safari or any browser app.

Purpose

Before	After
Screen.Recording.2021-08-09.at.01.17.17.mov	Screen.Recording.2021-08-09.at.00.13.51.mov

Current Tasks

install dep using yarn
check unused state lib/scroll-throttled.tsx
add tests global-header.tsx
tests lib/scroll-throttled.tsx (deleted)

netlify · 2021-08-08T18:46:03Z

✔️ Deploy Preview for wargabantuwarga ready!

🔨 Explore the source changes: 61b7899

🔍 Inspect the deploy log: https://app.netlify.com/sites/wargabantuwarga/deploys/61140c2b999f1f0007696cf9

😎 Browse the preview: https://deploy-preview-592--wargabantuwarga.netlify.app

zainfathoni · 2021-08-09T00:54:39Z

components/ui/scroll-throttled.tsx

@@ -0,0 +1,32 @@
+import { useState, useEffect } from "react";
+import { throttle } from "lodash";


Please don't use lodash, it's bloating up the dependency.
If you really need it, please make sure that you're tree-shaking it properly and only include the throttle code in the bundle.

However, based on our principles, do you think it's worth adding this throttle dependencies to the home page, @resir014? 😁

The bad thing from Lodash, it's not 100% tree-shake-able, since the code it self will use internal helpers.
So if we use it but only on small set, I will suggest to not use it completely

Yes @bungandy, please use a lighter alternative for the throttle function, or don't add it at all. As our principles state:

We must carefully consider any additional client-side libraries that we include on the website. Unless its benefits outweigh the trade-off that we have to pay, we should avoid adding the functionality.

We need to keep our website lightweight to allow for people with weak connection to easily access it, and lodash is not helping with that.

I think from the use case of throttle, it's replace-able with Debounce.
https://github.com/kawalcovid19/wargabantuwarga.com/blob/main/package.json#L44 we already have ts-debounce, maybe we can use it

Hi mas @zainfathoni and @resir014 understood mas and thanks for the inputs

mas @mazipan I have managed to use custom throttle I put to ~/lib/.. folder.
Or we can use throttle from @martinstark/throttle-ts alternatively, kindly please check on the next commit, thanks 🙏

Bundle stats for @martinstark/throttle-ts:

Bundle size
Minified: 725 B
Gzipped: 431 B

Download time
Slow 3G: 1ms
Emerging 4G: 60μs

This looks good. As @mazipan mentioned, we already have ts-debounce if you want to use libraries we have installed. If you can't use that, then feel free to use throttle-ts. @bungandy

@resir014 using throttle is better than debounce for header interaction I think mas, because debounce will always wait until the page is completely stopped when the page is scrolled whereas throttle will be executed in a certain time interval while the page being scrolled.

Throttle Debounce

Screen.Recording.2021-08-09.at.09.52.01.mov

Screen.Recording.2021-08-09.at.09.49.08.mov

reference

…artinstark

…artinstark

package.json

zainfathoni · 2021-08-09T03:12:06Z

components/layout/global-header.tsx

+  useDocumentScrollThrottled(
+    (callbackData: { previousScrollTop: Number; currentScrollTop: Number }) => {
+      const { previousScrollTop, currentScrollTop } = callbackData;
+      const isScrolledDown = previousScrollTop < currentScrollTop;
+      const isMinimumScrolled = currentScrollTop > MINIMUM_SCROLL;
+
+      setShouldShowShadow(currentScrollTop > 2);
+
+      setTimeout(() => {
+        setShouldHideHeader(isScrolledDown && isMinimumScrolled);
+      }, TIMEOUT_DELAY);
+    },
+  );


Please add more tests to cover these new cases that you're introducing, thanks!

@zainfathoni do you have any references where I can start making a test for this? thank you

@zainfathoni I have created a test for the throttle function. should I add test for when adding/removing classes on page scroll simulation?

zainfathoni · 2021-08-09T03:13:29Z

components/ui/scroll-throttled.tsx

+function useDocumentScrollThrottled(callback: {
+  (callbackData: { previousScrollTop: Number; currentScrollTop: Number }): void;
+  (arg0: { previousScrollTop: number; currentScrollTop: number }): void;
+}) {
+  const [, setScrollPosition] = useState(0);
+  let previousScrollTop = 0;
+
+  function handleDocumentScroll() {
+    const { scrollTop: currentScrollTop } = document.documentElement;
+
+    setScrollPosition((previousPosition) => {
+      previousScrollTop = previousPosition;
+      return currentScrollTop;
+    });
+
+    callback({ previousScrollTop, currentScrollTop });
+  }
+
+  const [handleDocumentScrollThrottled] = throttle(handleDocumentScroll, 150);
+
+  useEffect(() => {
+    window.addEventListener("scroll", handleDocumentScrollThrottled);
+
+    return () =>
+      window.removeEventListener("scroll", handleDocumentScrollThrottled);
+  }, []);
+}


If your new tests for the global-header.tsx is comprehensive enough, all these lines should have been covered.

@zainfathoni I've deleted this file and merged the function to global-header.tsx please check it again mas

zainfathoni · 2021-08-09T03:14:15Z

components/ui/scroll-throttled.tsx

+  (callbackData: { previousScrollTop: Number; currentScrollTop: Number }): void;
+  (arg0: { previousScrollTop: number; currentScrollTop: number }): void;
+}) {
+  const [, setScrollPosition] = useState(0);


What's the point of setting the state here if you're not using the value at all?

@zainfathoni I've deleted this file and merged the function to global-header.tsx please check it again mas

codecov · 2021-08-09T03:31:43Z

Codecov Report

Merging #592 (61b7899) into main (14b92a9) will decrease coverage by 0.03%.
The diff coverage is 76.00%.

@@            Coverage Diff             @@
##             main     #592      +/-   ##
==========================================
- Coverage   80.48%   80.44%   -0.04%     
==========================================
  Files         115      119       +4     
  Lines        1291     1355      +64     
  Branches      420      444      +24     
==========================================
+ Hits         1039     1090      +51     
- Misses        246      257      +11     
- Partials        6        8       +2

Impacted Files	Coverage Δ
components/home/homepage-emergency-cta.tsx	`100.00% <ø> (ø)`
components/home/homepage-welcome/dialog.tsx	`100.00% <ø> (ø)`
components/kontak-darurat/chatbot-section.tsx	`100.00% <ø> (ø)`
...nents/kontak-darurat/emergency-contact-section.tsx	`100.00% <ø> (ø)`
components/layout/page/page-content.tsx	`100.00% <ø> (ø)`
components/layout/page/page-header.tsx	`60.00% <0.00%> (ø)`
components/ui/alert/alert.tsx	`100.00% <ø> (ø)`
components/ui/alert/utils/helpers.ts	`100.00% <ø> (ø)`
pages/404.tsx	`100.00% <ø> (+100.00%)`	⬆️
pages/_error.tsx	`100.00% <ø> (+100.00%)`	⬆️
... and 28 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14b92a9...61b7899. Read the comment docs.

…a.com into bungandy/header-hides-shows-on-scroll

…scroll

mazipan · 2021-08-11T23:45:01Z

components/layout/global-header.tsx


 export function GlobalHeader() {
  const popoverButtonRef = useRef<HTMLButtonElement>(null);

  const [shouldHideHeader, setShouldHideHeader] = useState(false);
-  const [shouldShowShadow, setShouldShowShadow] = useState(false);
+  const [, setScrollPosition] = useState(0);
+  let previousScrollTop = 0;


You can try useRef to save the previous render value.

I don't think we need setScrollPosition in this case, just change to useRef

mazipan · 2021-08-11T23:46:39Z

components/layout/global-header.tsx

-      const { previousScrollTop, currentScrollTop } = callbackData;
-      const isScrolledDown = previousScrollTop < currentScrollTop;
-      const isMinimumScrolled = currentScrollTop > MINIMUM_SCROLL;
+    setScrollPosition((previousPosition) => {


We can not use same name for arguments and upper scope variables, it cause a confusing

feat: hides/shows header on scroll

aad26bf

zainfathoni requested changes Aug 9, 2021

View reviewed changes

perf: replace throttle vendor. remove lodash, add throttle-ts from @m…

c73d54e

…artinstark

bungandy requested a review from zainfathoni August 9, 2021 02:57

zainfathoni requested changes Aug 9, 2021

View reviewed changes

chore: add yarn

0acb88c

bungandy and others added 13 commits August 9, 2021 15:08

test: global-header

90cfdc2

Merge branch 'main' into bungandy/header-hides-shows-on-scroll

083b0d5

fix: global-header test

0b1b88f

Merge branch 'main' of https://github.com/kawalcovid19/wargabantuwarg…

9103601

…a.com into bungandy/header-hides-shows-on-scroll

Merge branch 'kawalcovid19:main' into bungandy/header-hides-shows-on-…

232fb66

…scroll

fix: global-header.tsx

95bb4e2

fix: global-header

4e86563

Merge branch 'main' into bungandy/header-hides-shows-on-scroll

523c5ea

chore: merge main branch

711570c

refactor: global-header test

dbacd10

chore: conflict with main

e666d2e

test: throttle function, delete lib/scroll-throttle

03895bb

test: test target 80.48% still working on it

61b7899

mazipan reviewed Aug 11, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: hides/shows header on scroll #592

feat: hides/shows header on scroll #592

bungandy commented Aug 8, 2021 •

edited

netlify bot commented Aug 8, 2021 •

edited

zainfathoni Aug 9, 2021

mazipan Aug 9, 2021

resir014 Aug 9, 2021 •

edited

mazipan Aug 9, 2021

bungandy Aug 9, 2021

resir014 Aug 9, 2021 •

edited

bungandy Aug 9, 2021

zainfathoni Aug 9, 2021

bungandy Aug 9, 2021

bungandy Aug 11, 2021

zainfathoni Aug 9, 2021

bungandy Aug 11, 2021

zainfathoni Aug 9, 2021

bungandy Aug 11, 2021

codecov bot commented Aug 9, 2021 •

edited

mazipan Aug 11, 2021

mazipan Aug 11, 2021

mazipan Aug 11, 2021

		@@ -0,0 +1,32 @@
		import { useState, useEffect } from "react";
		import { throttle } from "lodash";

feat: hides/shows header on scroll #592

Are you sure you want to change the base?

feat: hides/shows header on scroll #592

Conversation

bungandy commented Aug 8, 2021 • edited

Hides/shows header on scrolling a page

Why?

Objective

Purpose

Current Tasks

netlify bot commented Aug 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resir014 Aug 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

resir014 Aug 9, 2021 • edited

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

Choose a reason for hiding this comment

codecov bot commented Aug 9, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bungandy commented Aug 8, 2021 •

edited

netlify bot commented Aug 8, 2021 •

edited

resir014 Aug 9, 2021 •

edited

resir014 Aug 9, 2021 •

edited

codecov bot commented Aug 9, 2021 •

edited