Mobile Organized Content #79

Merged
w25tran merged 53 commits from feat/organized-content into main 2021-08-27 15:18:56 -04:00
Contributor
No description provided.
w25tran added 32 commits 2021-07-04 17:43:10 -04:00
w25tran added 1 commit 2021-07-04 17:45:55 -04:00
w25tran added 1 commit 2021-07-04 17:53:17 -04:00
w25tran added 1 commit 2021-07-05 13:04:55 -04:00
w25tran changed title from WIP: Mobile Organized Content to Mobile Organized Content 2021-07-05 13:08:57 -04:00
w25tran requested review from a3thakra 2021-07-05 13:09:51 -04:00
w25tran added 1 commit 2021-07-05 20:22:56 -04:00
w25tran added 1 commit 2021-07-07 18:06:37 -04:00
Owner

@w25tran can you update this branch?

@w25tran can you update this branch?
Author
Contributor

@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 2021-07-19 00:49:21 -04:00
@ -113,0 +125,4 @@
}
.hiddenBurger {
bottom: calc(-94rem / 16);
Owner

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 2021-07-19 01:19:57 -04:00
@ -138,0 +256,4 @@
);
};
function useOutsideAlerter(
Owner

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 2021-07-19 01:22:08 -04:00
@ -55,6 +73,15 @@ export function OrganizedContent(props: Props) {
</>
)}
</div>
<MobileWrapper
Owner

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 2021-07-19 01:34:48 -04:00
@ -138,0 +196,4 @@
const [prevScrollPos, setPrevScrollPos] = useState(0);
const [burgerVisible, setBurgerVisible] = useState(true);
const handleScroll = useDebounce(() => {
Owner

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 2021-07-19 01:36:15 -04:00
@ -138,0 +219,4 @@
return (
<div
className={
styles.burger + " " + (burgerVisible ? "" : styles.hiddenBurger)
Owner
`${styles.burger} ${burgerVisible ? "" : styles.hiddenBurger}`

is preferred.

```js `${styles.burger} ${burgerVisible ? "" : styles.hiddenBurger}` ``` is preferred.
w25tran marked this conversation as resolved
a3thakra reviewed 2021-07-19 01:39:59 -04:00
@ -175,0 +341,4 @@
return () => {
observer.disconnect();
};
}, [ref]);
Owner

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
Owner

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 :)
Owner

@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 2021-07-24 23:29:23 -04:00
a3thakra reviewed 2021-07-26 15:30:10 -04:00
@ -114,0 +134,4 @@
z-index: 10;
background: var(--light-blue-2);
border-radius: calc(5rem / 16);
transition: transform 0.6s ease-in-out;
Owner

0.3s instead? 0.6 feels very slow

0.3s instead? 0.6 feels very slow
w25tran marked this conversation as resolved
a3thakra reviewed 2021-07-26 15:38:57 -04:00
@ -66,3 +146,3 @@
}
function Nav({ sections, currentIndex, link: Link }: NavProps) {
function Nav({ sections, currentIndex, link: Link, pageTitle }: NavProps) {
Owner

Clicking on nav items should close the sidebar

Clicking on nav items should close the sidebar
w25tran marked this conversation as resolved
a3thakra reviewed 2021-07-26 15:41:03 -04:00
@ -113,1 +120,4 @@
@media only screen and (max-width: calc(768rem / 16)) {
.burger {
display: flex;
Owner

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 2021-07-26 21:32:53 -04:00
continuous-integration/drone/push Build is passing Details
217b62e69a
Styling changes from PR comments
w25tran added 1 commit 2021-07-26 22:20:06 -04:00
continuous-integration/drone/push Build is passing Details
7979c09d00
Make clicking NavItem close the hamburger menu
a3thakra reviewed 2021-07-26 23:01:21 -04:00
@ -84,6 +174,7 @@ function Nav({ sections, currentIndex, link: Link }: NavProps) {
className={classNames.join(" ")}
id={section.id}
key={section.id}
setMobileNavOpen={setMobileNavOpen}
Owner

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 2021-08-01 18:53:34 -04:00
@ -58,0 +100,4 @@
onClick={() => setMobileNavOpen(!mobileNavOpen)}
>
{/* this is copied from hamburger.svg with changed colors */}
<svg
Owner

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 2021-08-26 19:46:42 -04:00
continuous-integration/drone/push Build is failing Details
e1892c00b1
Mobile OC bug fixes
Navbar z-index was removed because we want the
OC menu to be above the Navbar while also having
the Navbar menu above the OC burger.
w25tran added 2 commits 2021-08-26 19:53:00 -04:00
continuous-integration/drone/push Build is failing Details
4fd4fd6987
Add back in navbar z-index
w25tran added 1 commit 2021-08-26 20:03:18 -04:00
continuous-integration/drone/push Build is passing Details
95cc1c10a6
Linting fixes
w25tran added 3 commits 2021-08-26 21:10:35 -04:00
w25tran added 1 commit 2021-08-26 21:12:06 -04:00
continuous-integration/drone/push Build is passing Details
245b1a6378
Merge branch 'main' into feat/organized-content
a3thakra added 1 commit 2021-08-27 15:02:48 -04:00
continuous-integration/drone/push Build is passing Details
83d6d73134
Change setMobileNavOpen type
a3thakra approved these changes 2021-08-27 15:03:07 -04:00
a3thakra approved these changes 2021-08-27 15:03:11 -04:00
w25tran merged commit 4237da76e3 into main 2021-08-27 15:18:56 -04:00
a3thakra deleted branch feat/organized-content 2021-08-27 15:20:18 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#79
No description provided.