-
Notifications
You must be signed in to change notification settings - Fork 2
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
IH Schedule Page 2025 #581
base: main
Are you sure you want to change the base?
Conversation
Deploy preview for irvinehacks-site-2025 ready!
|
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.
Functionality looks good, great even! Just quite a few notes on cleaning up code.
import Sprite3 from "@/assets/images/volunteer_sprite.png"; | ||
|
||
import styles from "./ShiftingCountdown.module.scss"; | ||
import Image from "next/image"; |
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.
Chore (repeating): Reorder imports to libraries, custom components/functions, and then stylesheets.
apps/site/src/app/(main)/schedule/components/ShiftingCountdown/ShiftingCountdown.tsx
Show resolved
Hide resolved
}} | ||
/> | ||
<div className="absolute top-[50%] right-14 max-md:hidden"> | ||
<Image src={LgStar} alt="*" className="w-[70px] left-14" /> |
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.
suggestion: use more descriptive alt text
return new Date(date.getFullYear(), date.getMonth(), date.getDate()); | ||
} | ||
|
||
// function getEventTomorrow(date: Date) { |
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.
chore: remove unused commented code
return <EventCard key={event.title} now={now} {...event} />; | ||
})} | ||
</Tabs.Content> | ||
<> |
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.
Chore: remove fragment and just return div
function scrollDir(action: string) { | ||
const ind = getCurrentDateIndex(); | ||
|
||
if (action === "left") { |
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.
suggestion: last 2 lines of the if statement could be extracted out as they happen in both cases.
Also, you could then use a ternary operator to calculate nextIndex
Draft, IP items include