-
Notifications
You must be signed in to change notification settings - Fork 166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: hides/shows header on scroll #592
base: main
Are you sure you want to change the base?
feat: hides/shows header on scroll #592
Conversation
✔️ 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 |
components/ui/scroll-throttled.tsx
Outdated
@@ -0,0 +1,32 @@ | |||
import { useState, useEffect } from "react"; | |||
import { throttle } from "lodash"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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? 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 |
components/layout/global-header.tsx
Outdated
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); | ||
}, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add more tests to cover these new cases that you're introducing, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zainfathoni do you have any references where I can start making a test for this? thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zainfathoni I have created a test for the throttle
function. should I add test for when adding/removing classes on page scroll simulation?
components/ui/scroll-throttled.tsx
Outdated
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); | ||
}, []); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If your new tests for the global-header.tsx
is comprehensive enough, all these lines should have been covered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zainfathoni I've deleted this file and merged the function to global-header.tsx
please check it again mas
components/ui/scroll-throttled.tsx
Outdated
(callbackData: { previousScrollTop: Number; currentScrollTop: Number }): void; | ||
(arg0: { previousScrollTop: number; currentScrollTop: number }): void; | ||
}) { | ||
const [, setScrollPosition] = useState(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the point of setting the state here if you're not using the value at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zainfathoni I've deleted this file and merged the function to global-header.tsx
please check it again mas
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
…a.com into bungandy/header-hides-shows-on-scroll
|
||
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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can try useRef
to save the previous render value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need setScrollPosition
in this case, just change to useRef
const { previousScrollTop, currentScrollTop } = callbackData; | ||
const isScrolledDown = previousScrollTop < currentScrollTop; | ||
const isMinimumScrolled = currentScrollTop > MINIMUM_SCROLL; | ||
setScrollPosition((previousPosition) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can not use same name for arguments and upper scope variables, it cause a confusing
Hides/shows header on scrolling a page
Why?
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
Screen.Recording.2021-08-09.at.01.17.17.mov
Screen.Recording.2021-08-09.at.00.13.51.mov
Current Tasks
yarn
lib/scroll-throttled.tsx
global-header.tsx
lib/scroll-throttled.tsx
(deleted)