-
Notifications
You must be signed in to change notification settings - Fork 1
Dominick/639 feat: add user account link to sidebar #651
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
5474bf6
641610e
80eee47
5ecc1b0
634d468
24d3425
af5c6b1
800457d
51151ad
57e258e
9347392
96bd584
0423909
15b023f
1065a79
bc7b68c
d855689
3f43c62
2dbfc34
177f43c
fa1f00b
61a2961
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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'; | ||
| import { | ||
| Drawer, | ||
| DrawerContent, | ||
|
|
@@ -29,6 +29,12 @@ export const Nav = (): JSX.Element => { | |
| const { logoutAccount } = useAuthContext(); | ||
| const [open, setOpen] = React.useState(false); | ||
|
|
||
| /** | ||
| * Handles closing the drawer. | ||
| * @returns {void} No return value. | ||
| */ | ||
| const handleClose = (): void => setOpen(false); | ||
|
|
||
| /** | ||
| * Handles the logout. | ||
| * @returns {Promise<void>} The logout promise. | ||
|
|
@@ -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"> | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: could go as far as making this NavLinkContainer component.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't quite understand what you mean here. Are you thinking |
||
| <li> | ||
| <Link | ||
| href="/account/settings" | ||
| className="underline underline-offset-4 hover:text-primary-muted transition-colors" | ||
| onClick={() => setOpen(false)} | ||
| data-testid="settings-link" | ||
| > | ||
| Settings | ||
| </Link> | ||
| </li> | ||
| <li> | ||
| <Link | ||
| data-testid="league-link" | ||
| href="/league/all" | ||
| className={cn( | ||
| 'underline underline-offset-4 hover:text-primary-muted transition-colors', | ||
| )} | ||
| onClick={() => setOpen(false)} | ||
| > | ||
| Leagues | ||
| </Link> | ||
| </li> | ||
| <NavLink | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nicely done!
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you! |
||
| href="/account/settings" | ||
| testId="settings-link" | ||
| onClose={handleClose} | ||
| > | ||
| Settings | ||
| </NavLink> | ||
| <NavLink | ||
| href="/league/all" | ||
| testId="league-link" | ||
| onClose={handleClose} | ||
| > | ||
| Leagues | ||
| </NavLink> | ||
| <NavLink | ||
| href="/account/profile" | ||
| testId="profile-link" | ||
| onClose={handleClose} | ||
| > | ||
| Profile | ||
| </NavLink> | ||
| <li> | ||
| <Button | ||
| className="p-0 text-base font-normal hover:text-primary-muted transition-colors text-white" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { render, screen, fireEvent } from '@testing-library/react'; | ||
| import NavLink from './NavLink'; | ||
|
|
||
| describe('NavLink componenet', () => { | ||
| it('renders link with href and testId', () => { | ||
| render( | ||
| <NavLink href="test/url" testId="test-link"> | ||
| Test Link | ||
| </NavLink>, | ||
| ); | ||
|
|
||
| const link = screen.getByTestId('test-link'); | ||
| expect(link).toBeInTheDocument(); | ||
| expect(link).toHaveAttribute('href', 'test/url'); | ||
| expect(link).toHaveTextContent('Test Link'); | ||
| }); | ||
|
|
||
| it('calls onClose when link is clicked', () => { | ||
| const handleClose = jest.fn(); | ||
| render( | ||
| <NavLink href="/test-url" testId="test-link" onClose={handleClose}> | ||
| Test Link | ||
| </NavLink>, | ||
| ); | ||
|
|
||
| const linkNav = screen.getByTestId('test-link'); | ||
| fireEvent.click(linkNav); | ||
| expect(handleClose).toHaveBeenCalledTimes(1); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| // Copyright (c) Gridiron Survivor. | ||
| // Licensed under the MIT License. | ||
|
|
||
| import React, { JSX } from 'react'; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Need a unit test for this new component.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. You should create a brand new NavLink unit test file. It should ensure that all the props that can change will change correctly.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @shashilo Latest changes address this i think |
||
| import Link from 'next/link'; | ||
| import { cn } from '@/utils/utils'; | ||
| interface NavLinkProps { | ||
| href: string; | ||
| testId: string; | ||
| children: React.ReactNode; | ||
| onClose?: () => void; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is onClose optional?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I may be misunderstanding, but my line of thought was it needed to be optional because |
||
| } | ||
|
|
||
| /** | ||
| * 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] - function to be called when the link is clicked. | ||
| * @returns {JSX.Element} The rendered navigation link. | ||
| */ | ||
| const NavLink = ({ | ||
| href, | ||
| testId, | ||
| children, | ||
| onClose, | ||
| }: NavLinkProps): JSX.Element => { | ||
| return ( | ||
| <li> | ||
| <Link | ||
| href={href} | ||
| data-testid={testId} | ||
| className={cn( | ||
| 'underline underline-offset-4 hover:text-primary-muted transition-colors', | ||
| )} | ||
| onClick={onClose} | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this was optional, this code should fail.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:" |
||
| > | ||
| {children} | ||
| </Link> | ||
| </li> | ||
| ); | ||
| }; | ||
|
|
||
| export default NavLink; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need a variable to call this useState directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was getting the following error if I didn't declare the useState directly:
Would you prefer this bit implemented differently?
