Mai/Fix-413-added in Link to logo #446
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 ↗︎
|
components/LogoNav/LogoNav.test.tsx
Outdated
| it('renders the logo and links to the /league/all page', () => { | ||
| render(<LogoNav />); | ||
|
|
||
| const logoImage = screen.getByAltText('Gridiron Survivor logo'); |
There was a problem hiding this comment.
Never grab a component by the alt text. This could change any time, thus failing your test very easily. Add a data-testid, then find the component by this attribute.
There was a problem hiding this comment.
Updated to grab by getByTestId instead.
components/LogoNav/LogoNav.tsx
Outdated
| className="w-20" | ||
| data-testid="logo-nav" | ||
| /> | ||
| <Link href="/league/all" passHref> |
There was a problem hiding this comment.
Per this Stackoverflow blog I was reading, it stated passHref is needed for a functional component per the nextjs documentation, but as I'm reading this again, it stated to 'pass it whenever you use a react component as a child' and for this logo instance, there are no children now that I think about it and will be removing the passHref.
components/LogoNav/LogoNav.tsx
Outdated
| <Image | ||
| src={logo} | ||
| alt="Gridiron Survivor logo" | ||
| width={1} |
There was a problem hiding this comment.
width and height of 1px?
There was a problem hiding this comment.
This is a good question. It seemed the changes were made prior to my change but let me check with the Figma design.
There was a problem hiding this comment.
Removed width, height, className, and priority and verified design specs adhere to Figma's design.
components/LogoNav/LogoNav.tsx
Outdated
| <Link href="/league/all"> | ||
| <Image | ||
| src={logo} | ||
| width={80} |
There was a problem hiding this comment.
How does this scale for the mobile view?
There was a problem hiding this comment.
Scale view perspective shows height={36} and width={80}.
components/LogoNav/LogoNav.tsx
Outdated
| /> | ||
| <Link href="/league/all"> | ||
| <Image | ||
| src={logo} |
There was a problem hiding this comment.
Alphabetize your attribute names.
components/LogoNav/LogoNav.test.tsx
Outdated
| expect(logoImage).toBeInTheDocument(); | ||
|
|
||
| const linkElement = screen.getByRole('link', { | ||
| name: /Gridiron Survivor logo/i, |
There was a problem hiding this comment.
General question, what's the forward-slashes and "i" do here?
There was a problem hiding this comment.
@ryandotfurrer it's regex to look for the specific word of 'Gridiron Survivor Logo'. Here's a great blog about the i in Regex: https://www.w3schools.com/jsref/jsref_regexp_i.asp. Please let me know if this makes sense :)
components/LogoNav/LogoNav.test.tsx
Outdated
| const logoImage = screen.getByTestId('logo-nav'); | ||
| expect(logoImage).toBeInTheDocument(); | ||
|
|
||
| const linkElement = screen.getByRole('link', { |
There was a problem hiding this comment.
Are we able to use the datatest-id of logo-nav here instead?
There was a problem hiding this comment.
The data-testid="logo-nav" is currently used for the Image tag. I like the idea of adding a data-testID to test the Link tag and will implement one shortly.
fixes #413
Screen.Recording.2024-07-20.at.12.39.47.PM.mov