Mobile Organized Content #79

Merged
w25tran merged 53 commits from feat/organized-content into main 2 years ago
There is no content yet.
w25tran added 32 commits 2 years ago
w25tran added 1 commit 2 years ago
w25tran added 1 commit 2 years ago
w25tran added 1 commit 2 years ago
w25tran changed title from WIP: Mobile Organized Content to Mobile Organized Content 2 years ago
w25tran requested review from a3thakra 2 years ago
w25tran added 1 commit 2 years ago
w25tran added 1 commit 2 years ago
Collaborator

@w25tran can you update this branch?

@w25tran can you update this branch?
Poster

@w25tran can you update this branch?

@a3thakra This is already up to date sorry, I forgot to change the descripton

> @w25tran can you update this branch? @a3thakra This is already up to date sorry, I forgot to change the descripton
a3thakra reviewed 2 years ago
}
.hiddenBurger {
bottom: calc(-94rem / 16);
Collaborator

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

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
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
);
};
function useOutsideAlerter(
Collaborator

This is giving off a very over-engineered vibe. @a258wang used a much more straightforward approach with the mobile navbar.

This is giving off a very over-engineered vibe. @a258wang used a much more straightforward approach with the mobile navbar.
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
</>
)}
</div>
<MobileWrapper
Collaborator

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

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
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
const [prevScrollPos, setPrevScrollPos] = useState(0);
const [burgerVisible, setBurgerVisible] = useState(true);
const handleScroll = useDebounce(() => {
Collaborator

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 the useEffect run on line 213, which will keep adding new scroll event listeners, which might cause a performance impact.

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 the `useEffect` run on line 213, which will keep adding new scroll event listeners, which might cause a performance impact.
a3thakra marked this conversation as resolved
a3thakra reviewed 2 years ago
return (
<div
className={
styles.burger + " " + (burgerVisible ? "" : styles.hiddenBurger)
Collaborator
`${styles.burger} ${burgerVisible ? "" : styles.hiddenBurger}`

is preferred.

```js `${styles.burger} ${burgerVisible ? "" : styles.hiddenBurger}` ``` is preferred.
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
return () => {
observer.disconnect();
};
}, [ref]);
Collaborator

it might be a better idea to make ref.current a dependency, instead of ref.

it might be a better idea to make ref.current a dependency, instead of ref.
w25tran marked this conversation as resolved
Collaborator

Great job William!! 🎉

I have some architectural nitpicks, let's pair on it to make it a bit more simpler and cleaner :)

Great job William!! :tada: I have some architectural nitpicks, let's pair on it to make it a bit more simpler and cleaner :)
Collaborator

@w25tran can you update this branch?

@a3thakra This is already up to date sorry, I forgot to change the descripton

By update, I meant either rebase it, or merge origin/main into this branch.

> > @w25tran can you update this branch? > > @a3thakra This is already up to date sorry, I forgot to change the descripton By update, I meant either rebase it, or merge `origin/main` into this branch.
w25tran added 2 commits 2 years ago
a3thakra reviewed 2 years ago
z-index: 10;
background: var(--light-blue-2);
border-radius: calc(5rem / 16);
transition: transform 0.6s ease-in-out;
Collaborator

0.3s instead? 0.6 feels very slow

0.3s instead? 0.6 feels very slow
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
}
function Nav({ sections, currentIndex, link: Link }: NavProps) {
function Nav({ sections, currentIndex, link: Link, pageTitle }: NavProps) {
Collaborator

Clicking on nav items should close the sidebar

Clicking on nav items should close the sidebar
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
@media only screen and (max-width: calc(768rem / 16)) {
.burger {
display: flex;
Collaborator

you can just merge this into the class below

you can just merge this into the class below
w25tran marked this conversation as resolved
w25tran added 1 commit 2 years ago
217b62e69a Styling changes from PR comments
w25tran added 1 commit 2 years ago
7979c09d00 Make clicking NavItem close the hamburger menu
a3thakra reviewed 2 years ago
className={classNames.join(" ")}
id={section.id}
key={section.id}
setMobileNavOpen={setMobileNavOpen}
Collaborator

Instead of passing it into the Link component, we should wrap the things inside a link with a div.

Instead of passing it into the `Link` component, we should wrap the things inside a link with a `div`.
w25tran marked this conversation as resolved
a3thakra reviewed 2 years ago
onClick={() => setMobileNavOpen(!mobileNavOpen)}
>
{/* this is copied from hamburger.svg with changed colors */}
<svg
Collaborator

Can you separate it out to its own function within the same file?

Can you separate it out to its own function within the same file?
w25tran marked this conversation as resolved
w25tran added 4 commits 2 years ago
e1892c00b1 Mobile OC bug fixes
w25tran added 2 commits 2 years ago
4fd4fd6987 Add back in navbar z-index
w25tran added 1 commit 2 years ago
95cc1c10a6 Linting fixes
w25tran added 3 commits 2 years ago
w25tran added 1 commit 2 years ago
245b1a6378 Merge branch 'main' into feat/organized-content
a3thakra added 1 commit 2 years ago
83d6d73134 Change setMobileNavOpen type
a3thakra approved these changes 2 years ago
a3thakra approved these changes 2 years ago
w25tran merged commit 4237da76e3 into main 2 years ago
w25tran referenced this issue from a commit 2 years ago
a3thakra deleted branch feat/organized-content 2 years ago

Reviewers

a3thakra approved these changes 2 years ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 4237da76e3.
Sign in to join this conversation.
No reviewers
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…
There is no content yet.