Dominick/639 feat: add user account link to sidebar #651
Conversation
|
Gridiron Survivor Application
Project name: Gridiron Survivor Application
Only deployments on the production branch are activated automatically. If you'd like to activate this deployment, navigate to your deployments. Learn more about Appwrite Function deployments.
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Do we want any tests for this change? I see a data-testid added but no associated test. |
|
@mhchen I think that makes a lot of sense. I'm feeling up to it. Just to clarify, this @braydoncoyer That's another question I had! The other links have tests written for them in the |
If you touch it, you test it. |
When you come acrosss items that are repetitive, you should always look into componetizing them. It's easier to maintain and the code is only in 1 place. |
Great, thanks for the clarification. I'll get right on it! |
pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
Why was this file changed?
There was a problem hiding this comment.
Because I'm not the brightest tool in the shed.
It's a mistake that's since been reverted.
…kage-lock.json This reverts commit 800457d.
components/Nav/Nav.tsx
Outdated
| @@ -6,7 +6,7 @@ import React, { JSX } from 'react'; | |||
| import LogoNav from '../LogoNav/LogoNav'; | |||
| import { Menu } from 'lucide-react'; | |||
| import { Button } from '../Button/Button'; | |||
| import Link from 'next/link'; | |||
| import NavLink from '../NavLink/NavLink'; // Adjust the import path as necessary | |||
| * Handles closing the drawer. | ||
| * @returns {void} No return value. | ||
| */ | ||
| const handleClose = (): void => setOpen(false); |
There was a problem hiding this comment.
Why do we need a variable to call this useState directly?
There was a problem hiding this comment.
I was getting the following error if I didn't declare the useState directly:
Would you prefer this bit implemented differently?
| Leagues | ||
| </Link> | ||
| </li> | ||
| <NavLink |
| href: string; | ||
| testId: string; | ||
| children: React.ReactNode; | ||
| onClose?: () => void; |
There was a problem hiding this comment.
Why is onClose optional?
There was a problem hiding this comment.
I may be misunderstanding, but my line of thought was it needed to be optional because onClose it's a function that's toggled on click. The nature of the toggle indicating that the onClose function is always 'provided'/there.
components/NavLink/NavLink.tsx
Outdated
|
|
||
| /** | ||
| * Renders a navigation link. | ||
| * @param {NavLinkProps} props - The properties for the navigation link. |
There was a problem hiding this comment.
nit: Call out all the props
There was a problem hiding this comment.
Something like this format? This example doesn't handle other nits.
/**
* Renders a navigation link.
* @param {string} props.href - The URL the link points to.
* @param {string} props.testId - The test ID for the link.
* @param {React.ReactNode} props.children - The content to be rendered inside the link.
* @param {Function} [props.onClose] - Optional function to be called when the link is clicked.
* @returns {JSX.Element} The rendered navigation link.
*/
| className={cn( | ||
| 'underline underline-offset-4 hover:text-primary-muted transition-colors', | ||
| )} | ||
| onClick={onClose} |
There was a problem hiding this comment.
If this was optional, this code should fail.
There was a problem hiding this comment.
Oh that's true. If it was optional, would the better way of handling it defining a variable that states its conditional and then testing that? Something like:"
const NavLink = ({
href,
testId,
children,
onClose,
}: NavLinkProps): JSX.Element => {
const handleClick = () => {
if (onClose) {
onClose();
}
};
return (
<a href={href} data-testid={testId} onClick={handleClick}>
{children}
</a>
);
};
| @@ -69,28 +75,27 @@ export const Nav = (): JSX.Element => { | |||
| <DrawerTitle data-testid="title">Gridiron Survivor</DrawerTitle> | |||
| </DrawerHeader> | |||
| <ul className="m-0 flex flex-col gap-1 p-0"> | |||
There was a problem hiding this comment.
nit: could go as far as making this NavLinkContainer component.
There was a problem hiding this comment.
I don't quite understand what you mean here. Are you thinking NavLinkContainer would be a new component leveraging the <Drawer/> component?
| // Copyright (c) Gridiron Survivor. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import React, { JSX } from 'react'; |
There was a problem hiding this comment.
Need a unit test for this new component.
There was a problem hiding this comment.
Ah so I should take the relevant tests form the original nav.tsx and move them into a new NavList.test file? That makes sense.
There was a problem hiding this comment.
No. You should create a brand new NavLink unit test file. It should ensure that all the props that can change will change correctly.
There was a problem hiding this comment.
@shashilo Latest changes address this i think
| // Copyright (c) Gridiron Survivor. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import React, { JSX } from 'react'; |
There was a problem hiding this comment.
No. You should create a brand new NavLink unit test file. It should ensure that all the props that can change will change correctly.
…ttps://github.com/LetsGetTechnical/gridiron-survivor into dominick/639-feat-add-user-account-link-to-sidebar
Added a link to navbar for the account/profile route.
Component view:
Page View