Mobile Organized Content #79
Merged
w25tran
merged 53 commits from feat/organized-content
into main
2 years ago
Loading…
Reference in new issue
There is no content yet.
Delete Branch 'feat/organized-content'
Deleting a branch is permanent. It CANNOT be undone. Continue?
WIP: Mobile Organized Contentto Mobile Organized Content 2 years ago@w25tran can you update this branch?
@a3thakra This is already up to date sorry, I forgot to change the descripton
}
.hiddenBurger {
bottom: calc(-94rem / 16);
You should use
transform: translateY
instead of changing the bottom property because it more performant. https://stackoverflow.com/questions/7108941/css-transform-vs-position/53892597);
};
function useOutsideAlerter(
This is giving off a very over-engineered vibe. @a258wang used a much more straightforward approach with the mobile navbar.
</>
)}
</div>
<MobileWrapper
We should not have a separate component just for mobile unless it's absolutely necessary. We can wrap the desktop Nav with something which till be much better, and allow us to reuse the same component as the desktop
const [prevScrollPos, setPrevScrollPos] = useState(0);
const [burgerVisible, setBurgerVisible] = useState(true);
const handleScroll = useDebounce(() => {
We might need to use
useDebounce(useCallback(() => { ...the function...}, [deps]))
here.Not doing it will make the inline function change its reference on every render, which will change the reference of
handleScroll
on every render, which will make theuseEffect
run on line 213, which will keep adding new scroll event listeners, which might cause a performance impact.return (
<div
className={
styles.burger + " " + (burgerVisible ? "" : styles.hiddenBurger)
is preferred.
return () => {
observer.disconnect();
};
}, [ref]);
it might be a better idea to make ref.current a dependency, instead of ref.
Great job William!! 🎉
I have some architectural nitpicks, let's pair on it to make it a bit more simpler and cleaner :)
By update, I meant either rebase it, or merge
origin/main
into this branch.z-index: 10;
background: var(--light-blue-2);
border-radius: calc(5rem / 16);
transition: transform 0.6s ease-in-out;
0.3s instead? 0.6 feels very slow
}
function Nav({ sections, currentIndex, link: Link }: NavProps) {
function Nav({ sections, currentIndex, link: Link, pageTitle }: NavProps) {
Clicking on nav items should close the sidebar
@media only screen and (max-width: calc(768rem / 16)) {
.burger {
display: flex;
you can just merge this into the class below
className={classNames.join(" ")}
id={section.id}
key={section.id}
setMobileNavOpen={setMobileNavOpen}
Instead of passing it into the
Link
component, we should wrap the things inside a link with adiv
.onClick={() => setMobileNavOpen(!mobileNavOpen)}
>
{/* this is copied from hamburger.svg with changed colors */}
<svg
Can you separate it out to its own function within the same file?
4237da76e3
into main 2 years agoReviewers
4237da76e3
.