-
Notifications
You must be signed in to change notification settings - Fork 1
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
Layout work and homepage wip #39
Conversation
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.
Nice job on getting all of this initial setup stuff working!
I suggested a few super small changes, proposed a type modification, and added a couple of other considerations. Let me know if you have any questions/thoughts 👍
src/app/components/Footer.tsx
Outdated
const isActive = pathname === href; | ||
return ( | ||
(<Link href={href} passHref className={`usa-nav__link`} onClick={onClick}> | ||
<span className={(isActive && "navbar-item-active") + ' ' + styles.navbarItemText}>{text}</span> |
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've used classnames in a past project to help make this sort of thing a bit cleaner to work this. Probably not necessary yet but could be useful down the road!
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.
Never used it, but we should!
https://app.zenhub.com/workspaces/dibbs-experience-team-66995abeef3d740287bfa9c8/issues/gh/cdcgov/phdi/2811
src/app/components/Footer.tsx
Outdated
</Link>, | ||
<NavigationLink key="two" href={`${basePath}/`} text='Home' onClick={onClick} />, | ||
<NavigationLink key="two" href='/our-products' text='Our products' onClick={onClick} />, | ||
<NavigationLink key="two" href='/our-products' text='Case studies' onClick={onClick} />, |
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.
Here's an example of the minor change I mentioned if we modify the NavigationLinkProps
type.
<NavigationLink key="two" href='/our-products' text='Case studies' onClick={onClick} />, | |
<NavigationLink key="two" href='/our-products' onClick={onClick}> | |
Case studies | |
</NavigationLink>, |
src/app/components/Footer.tsx
Outdated
</>; | ||
} | ||
|
||
function NavigationLink({ href, text, onClick }: NavigationLinkProps) { |
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.
(Related to NavigationLinkProps
change)
function NavigationLink({ href, text, onClick }: NavigationLinkProps) { | |
function NavigationLink({ href, children, onClick }: NavigationLinkProps) { |
src/app/components/Footer.tsx
Outdated
const isActive = pathname === href; | ||
return ( | ||
(<Link href={href} passHref className={`usa-nav__link`} onClick={onClick}> | ||
<span className={(isActive && "navbar-item-active") + ' ' + styles.navbarItemText}>{text}</span> |
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.
(Related to NavigationLinkProps
change)
<span className={(isActive && "navbar-item-active") + ' ' + styles.navbarItemText}>{text}</span> | |
<span className={(isActive && "navbar-item-active") + ' ' + styles.navbarItemText}>{children}</span> |
src/app/components/NavbarUSWDS.tsx
Outdated
import React from "react"; | ||
import styles from "../styles/Home.module.scss"; | ||
|
||
interface NavigationLinkProps { |
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 think we could reuse a lot of what's in the Footer
component and a lot of my same comments could apply here too, so I'll mostly skip this file for now 😄 I'll leave it up to you whether we'd like to make changes here now or do some clean up in a different PR.
@@ -0,0 +1,282 @@ | |||
|
|||
.productOfferingsLink { |
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 know you called the CSS out specifically but I'm good with what's here for now 👍 We can likely do some clean up as we keep making progress but I don't know that we necessarily need to tackle that in this PR unless we'd like to.
Co-authored-by: Jake Wheeler <[email protected]>
Co-authored-by: Jake Wheeler <[email protected]>
Co-authored-by: Jake Wheeler <[email protected]>
src/app/components/Header.tsx
Outdated
<Link | ||
href='/' | ||
key="one" | ||
className={`usa-nav__link ${styles.homeNavItem}`} | ||
onClick={onClick}> | ||
|
||
<span className={styles.navbarItemText}>Home</span> | ||
|
||
</Link>, |
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.
Should we delete this entirely? I noticed the styling is set to display: none
and the Figma design doesn't include Home
in the header. Is there a reason we should keep it and keep it hidden?
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.
Yeah, this is leftover from the old site.
src/app/components/Header.tsx
Outdated
<span className={styles.navbarItemText}>Home</span> | ||
|
||
</Link>, | ||
( |
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.
This list should work no problem without these parentheses! We could get prettier added to the project so we don't ever have to worry about formatting going forward? 🤔
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.
yes, done!
src/app/components/Footer.tsx
Outdated
|
||
</Link>, | ||
( | ||
<NavigationLink key="two" href={`${basePath}/`} onClick={onClick}> |
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.
We should be able to get rid of the basePath
in these URLs. I just noticed the footer's links are doubling up on it. For example, when I click on Our products
I get a URL like this: http://localhost:3000/dibbs-site/dibbs-site/our-products
The links in the header seem to be correct and work fine 👍
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.
yeah, these are basically placeholders, but i fixed them
src/app/components/Header.tsx
Outdated
</NavigationLink> | ||
), | ||
( | ||
<NavigationLink key="two" href='/our-products' onClick={onClick}> |
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.
Should this href
be /case-studies
(or something along those lines)? Looks like this could apply to Footer
as well. Sorry I missed this the first time around 🫣
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.
same, fixed
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.
LGTM, nice work!
This adds some layout components and i've started working on the homepage