Mobile Organized Content #79
No reviewers
Labels
No Label
a11y
Backlog
Blocked
Bug
Content
Dependencies
Design
Feature Request
Good First Issue
In Progress
Performance
Priority - High
Priority - Low
Priority - Medium
Untriaged
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: www/www-new#79
Loading…
Reference in New Issue
No description provided.
Delete Branch "feat/organized-content"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
WIP: Mobile Organized Contentto Mobile Organized Content@w25tran can you update this branch?
@a3thakra This is already up to date sorry, I forgot to change the descripton
@ -113,0 +125,4 @@
}
.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@ -138,0 +256,4 @@
);
};
function useOutsideAlerter(
This is giving off a very over-engineered vibe. @a258wang used a much more straightforward approach with the mobile navbar.
@ -55,6 +73,15 @@ export function OrganizedContent(props: Props) {
</>
)}
</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
@ -138,0 +196,4 @@
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.@ -138,0 +219,4 @@
return (
<div
className={
styles.burger + " " + (burgerVisible ? "" : styles.hiddenBurger)
is preferred.
@ -175,0 +341,4 @@
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.@ -114,0 +134,4 @@
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
@ -66,3 +146,3 @@
}
function Nav({ sections, currentIndex, link: Link }: NavProps) {
function Nav({ sections, currentIndex, link: Link, pageTitle }: NavProps) {
Clicking on nav items should close the sidebar
@ -113,1 +120,4 @@
@media only screen and (max-width: calc(768rem / 16)) {
.burger {
display: flex;
you can just merge this into the class below
@ -84,6 +174,7 @@ function Nav({ sections, currentIndex, link: Link }: NavProps) {
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
.@ -58,0 +100,4 @@
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?