Conversation
|
Deploy preview for spring-hackathon-haven ready! ✅ Preview Built with commit 3534484. |
tudi2d
left a comment
tudi2d
left a comment
There was a problem hiding this comment.
Just quickly went over it. Looks really cool & see the review only as suggestions - nothing critical. Happy to ship & iterate.
Remember to rebase the branch before merging :)
| <Route path="/projects/2025" element={<Projects />} /> | ||
| <Route path="/projects/2025/:projectId" element={<Projects />} /> |
There was a problem hiding this comment.
You could use optional segments here https://reactrouter.com/6.30.0/route/route#optional-segments
| interface NavbarProps { | ||
| backgroundColor?: string; | ||
| } | ||
|
|
||
| const Navbar = ({ backgroundColor = "bg-transparent" }: NavbarProps) => { |
There was a problem hiding this comment.
| interface NavbarProps { | |
| backgroundColor?: string; | |
| } | |
| const Navbar = ({ backgroundColor = "bg-transparent" }: NavbarProps) => { | |
| interface NavbarProps { | |
| headerClass?: string; | |
| } | |
| const Navbar = ({ headerClass = "bg-transparent" }: NavbarProps) => { |
| <header | ||
| className={`fixed top-0 left-0 w-full z-50 transition-all duration-300 ${ | ||
| scrolled ? "bg-white/90 backdrop-blur-sm shadow-sm" : "bg-transparent" | ||
| scrolled ? "bg-white/90 backdrop-blur-sm shadow-sm" : backgroundColor |
There was a problem hiding this comment.
| scrolled ? "bg-white/90 backdrop-blur-sm shadow-sm" : backgroundColor | |
| scrolled ? "bg-white/90 backdrop-blur-sm shadow-sm" : headerClass |
There was a problem hiding this comment.
Questionable here anyways as it's only added on !scrolled & otherwise the background is static
| <div className="container mx-auto px-4 py-4 flex items-center justify-between"> | ||
| {/* Logo */} | ||
| <a href="#" className="font-bold text-xl text-springBlue"> | ||
| <Link to="/" className="font-bold text-xl text-springBlue"> |
There was a problem hiding this comment.
I believe this shouldn't have any difference as we are using anchors here, right? There won't be any rendering optimizations of react router anyways & react router also uses the browser history.
Nothing critical, but moving from anchor to button elements is not great for accessibility:)
| ref={ref} | ||
| sideOffset={sideOffset} | ||
| className={cn( | ||
| "z-50 overflow-hidden rounded-md border bg-popover px-3 py-1.5 text-sm text-popover-foreground shadow-md animate-in fade-in-0 zoom-in-95 data-[state=closed]:animate-out data-[state=closed]:fade-out-0 data-[state=closed]:zoom-out-95 data-[side=bottom]:slide-in-from-top-2 data-[side=left]:slide-in-from-right-2 data-[side=right]:slide-in-from-left-2 data-[side=top]:slide-in-from-bottom-2", |
| const projectsByCase = projects.reduce((acc, project) => { | ||
| if (!acc[project.case]) { | ||
| acc[project.case] = []; | ||
| } | ||
| acc[project.case].push(project); | ||
| return acc; | ||
| }, {} as Record<string, Project[]>); | ||
|
|
||
| // Sort projects within each case | ||
| Object.keys(projectsByCase).forEach(caseKey => { | ||
| projectsByCase[caseKey].sort((a, b) => { | ||
| // First sort by placement (1st place, then 2nd place) | ||
| if (a.placement && b.placement) { | ||
| return a.placement - b.placement; | ||
| } | ||
| if (a.placement) return -1; | ||
| if (b.placement) return 1; | ||
|
|
||
| // Then sort challenge winners | ||
| const aHasChallenges = a.challenges && a.challenges.length > 0; | ||
| const bHasChallenges = b.challenges && b.challenges.length > 0; | ||
| if (aHasChallenges && !bHasChallenges) return -1; | ||
| if (!aHasChallenges && bHasChallenges) return 1; | ||
|
|
||
| // Finally sort alphabetically by name | ||
| return a.name.localeCompare(b.name); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This could be optimized to not do everything on each rerender & go over all projects twice + nested sort. It shouldn't be to bad for the amount of projects we will have, but not sure how they will be loaded & if there might be a few more rerenders in the future.
| const projectsByChallenge = projects.reduce((acc, project) => { | ||
| project.challenges?.forEach(challenge => { | ||
| if (!acc[challenge.sponsoredBy]) { | ||
| acc[challenge.sponsoredBy] = []; | ||
| } | ||
| acc[challenge.sponsoredBy].push(project); | ||
| }); | ||
| return acc; | ||
| }, {} as Record<string, Project[]>); | ||
|
|
| useEffect(() => { | ||
| document.documentElement.style.backgroundColor = "#fff"; | ||
| return () => { | ||
| document.documentElement.style.backgroundColor = ""; | ||
| }; | ||
| }, []); |
| {sortedCases.map((caseKey) => ( | ||
| <section key={caseKey}> | ||
| <div className="flex items-center gap-3 mb-4"> | ||
| <a href={cases[caseKey as keyof typeof cases].sponsorUrl} target="_blank" rel="noopener noreferrer"> | ||
| <img | ||
| src={cases[caseKey as keyof typeof cases].logo} | ||
| alt={`${cases[caseKey as keyof typeof cases].name} logo`} | ||
| className={cases[caseKey as keyof typeof cases].logoClass} | ||
| /> | ||
| </a> | ||
| </div> | ||
| <p className="text-gray-600 mb-4">{cases[caseKey as keyof typeof cases].description}</p> |
There was a problem hiding this comment.
types can be optimized here & everywhere bellow - looks funny though :D
| <div className="bg-white z-0 mt-16"> | ||
| <div className="container mx-auto py-12 px-4 min-h-screen"> | ||
| <header className="text-center mb-16"> | ||
| <h1 className="text-4xl font-bold mb-4">Projects from CDTM Hacks 2025</h1> |
2 participants
Uh oh!
There was an error while loading. Please reload this page.