Feat(HomePage): Create Home Page #18
Conversation
section header 👍
There was a problem hiding this comment.
Things mostly look great, I left a few comments just about best practice going forward and the advantages which we can talk about furthur at a later date, a few things are just adding comments, and then a few are just small style changes. Everything mostly looks great, great work Kara.
|
Closes #6 |
components/Button.tsx
Outdated
| }) => { | ||
| const classes = `${className} ${type} ${styles.button}`; | ||
| const content = label || children; | ||
| if (props.image) { |
There was a problem hiding this comment.
one refactor here which will make things better for maintence make this:
const content = <>
{(props.image == undefined ? <></> : <div className={styles.imageWrapper}>
<Image src={props.image.src} alt={props.image.altText} fill={true} />
</div>)}
{props.label == undefined ??<></> : (<span className={styles.buttonText}>{props.label}</span>)}
</>
components/Button.tsx
Outdated
| }) => { | ||
| const classes = `${className} ${type} ${styles.button}`; | ||
| const content = label || children; | ||
| if (props.image) { | ||
| return ( |
There was a problem hiding this comment.
Then we can get rid of the route this.
| )} | ||
| </Link> | ||
| ); | ||
| } | ||
|
|
||
| return href ? ( |
There was a problem hiding this comment.
and we can just pass the content in down here :) which means one point of maintence.
config.d.ts
Outdated
| email: string; | ||
| tagline: string; | ||
| footer: Footer; | ||
| HomeSection: HomeSection; |
There was a problem hiding this comment.
why is there a homeSection and a homeSections? and why is the capitalization different for this one?
layouts/LatestNews.tsx
Outdated
| const formatDate = (date: Date): string => { | ||
| const day = date.getDate(); | ||
|
|
||
| const getDaySuffix = (day: number): string => { |
There was a problem hiding this comment.
is there no toLocale that outputs this right? Maybe check out the moment time library from npm it might have what we want. This approach isn't the best for localization imo, which is why i think a time library might be better.
If you can't find something suitable we can keep this logic so maybe only spend like 15 mins looking into moment or the default localization api's
layouts/Section.tsx
Outdated
| (typeof TextSectionStyle)[keyof typeof TextSectionStyle]; | ||
|
|
||
| interface SectionProps { | ||
| sectionConfig: typeof HomeSection; |
There was a problem hiding this comment.
Hmms, You might need to make a types.ts and import the types in config.d.ts from there, I don't like how you have to say typeof HomeSection here im pretty sure it means something different. Can you just verify this is a strict typecheck, and if not make that change.
| index: number; | ||
| } | ||
|
|
||
| const getStyle = (index: number) => { |
layouts/Section.tsx
Outdated
| const { sectionStyle, location, newsSectionStyle, textSectionStyle } = | ||
| getStyle(index); | ||
|
|
||
| let content: React.ReactNode; |
There was a problem hiding this comment.
can we break this out into a getContent function, this would avoid the mutability, which is a good practice and honestly makes the code a lot better. also can we use a switch statement instead of if statements.
| // Internal Components | ||
| import NavBar from '../components/NavBar'; | ||
| import EventBanner from '../components/EventBanner'; | ||
| import HeroBanner from '../components/HeroBanner'; | ||
| import Footer from '../components/Footer'; | ||
| import Section from '../layouts/Section'; |
There was a problem hiding this comment.
I think we should rename this to HomeSection jus tso we dont run into any naming issues between our section and the html version.
- Refactor code in button - Remove unnecessary section - Better exports - Use Moment.js for timezone - change name of Section to HomeSection - unnecessary keyof typeof stuff removed
Link to Preview: https://club-website-c1tjlgepg-karas-projects-e1ba5d81.vercel.app
Needed Additions
Additional notes: