Issue 5 about us/committee page #57
Conversation
Created a basic committee model, implementation of the enum may be different to what is desired so can easily be redone. Migrations haven't been made yet because if the issue I mentioned on discord.
- Increase title size to text-5xl/6xl for better prominence - Change underline color from gray to white to match design - Apply pixelated font (jersey10) to paragraph text - Replace text placeholder with ImageIcon component - Add second paragraph content to match design spec - Improve responsive sizing and spacing
- Add "About Us" section with styled title, underline, and content - Implement "Our Committee" section with full-width layout - Add committee member cards with portrait frames - Update color scheme to match design: * About Us title: #9B9BDE (light purple) * Underline: #5E5ECC (blue/purple) * Image placeholder: #C5C5E8 (light purple) * Main background: #090A19 (dark navy) * Committee section: #1B1F4C (purple-navy) - Make committee section full-width with flat edges - Increase portrait frame size to 180x185px - Add proper spacing between title and portraits - Style committee member names and roles with badges
- Revised the Committee model's role attribute to actually work now, turning it into a CharField with the roles as choices. - Added a __str__ method for the Committee model, where a committee object is represented by the name of its connected member object - Made all migrations for the Committee Model - Registered the model on the admin.py file, it seems fully functional on the admin dashboard
- Fixed name display format and text alignment (left-aligned) - Fixed background colors (#1B1F4C) with proper text wrapping - Fixed alignment between portraits and text labels
…to issue-5-About_us/committee_page
- Changed navbar positioning from sticky to fixed - Added left-0 right-0 to anchor navbar to viewport edges - Added z-50 to ensure navbar stays above other content - Added pt-24 padding to content wrapper to prevent content from going behind fixed navbar - Removed unnecessary fragment wrapper in navbar component
- Added a get_member() function to the Committee model which returns the object's corresponding member object - Added CommitteeAPIView which retrieves the list of members that are on the committee, in a specific order - Added this view to the committee page URL at 'about/' - Created useCommittee hook which fetches the response from the CommitteeAPIView and has appropriate error handling - Modified the about page to use this hook and dynamically use the response to display the committee members, with appropriate error handling
This reverts commit 964b84a.
- Gave the Navbar a z-index of 100 so it is now fully up to date with main
- Made two migrations to the Committee Model, setting on_delete to CASCADE for the id field and setting unique to True for the role field. - Made a series of tests for the Committee model
- Undid the removal of game_dev_club_logo.svg, next.svg and vercel.svg from client/public - Undid a change to CORS_ALLOWED_ORIGINS in the settings.py file
-Replaced the placeholder ImageIcon used for the committee portraits with Images sourced by their profile_picture attribute
- Replaced the topRow and bottomRow types with arrays of type ApiMember, imported from the useCommittee hook, rather than an unsafe any[] type. - Did formatting for prettier and flake8 (not fully done for flake8)
laurenpudz
left a comment
laurenpudz
left a comment
There was a problem hiding this comment.
Looking good! Could you please make sure you run prettier, eslint and flake8? Those checks all seem to failing atm.
| } | ||
|
|
||
| return ( | ||
| <main className="min-h-screen bg-[#090A19]"> |
There was a problem hiding this comment.
Please change all hex codes to one of our custom colour variables. Most of the commonly used figma colours should have a corresponding variable.
| </div> | ||
| <div className="relative aspect-[4/3] w-full flex-shrink-0 overflow-hidden rounded-2xl bg-[#C5C5E8] md:w-96 lg:w-[512px]"> | ||
| <div className="flex h-full w-full items-center justify-center"> | ||
| <ImageIcon className="h-32 w-32 text-white/40" /> |
There was a problem hiding this comment.
Could you please use the next Image component here with the existing placeholder image we already have in the repo instead? It will make it easier for the game dev committee to swap in a different image when they feel ready.
| <p>Lorem ipsum dolor such and such I can't remember the rest.</p> | ||
| </div> | ||
| </div> | ||
| <div className="relative aspect-[4/3] w-full flex-shrink-0 overflow-hidden rounded-2xl bg-[#C5C5E8] md:w-96 lg:w-[512px]"> |
There was a problem hiding this comment.
Is there a tailwindCSS utility class you could use here instead of w-512px? If you need a larger width, there are -xl and -2xl classes that will go larger than the numbered classes. Could you also please try to replace as many arbitrary px classes as you can throughout the PR please :)
| topRow.push({ | ||
| name: "Loading...", | ||
| pronouns: "", | ||
| profile_picture: "", | ||
| about: "", | ||
| }); | ||
| } else { | ||
| bottomRow.push({ | ||
| name: "Loading...", | ||
| pronouns: "", | ||
| profile_picture: "", | ||
| about: "", | ||
| }); |
There was a problem hiding this comment.
It's not great to pass an empty string to the src prop of an Image. "" is not a valid image source and can cause performance issues and a range of console errors. You could pass the path to the placeholder image here instead? You may also be able to use null instead of an empty string (not 100% though).
| const errorMessage = | ||
| error?.response?.status === 404 | ||
| ? "Committee Page not found." | ||
| : "Failed to load Committee Page."; |
There was a problem hiding this comment.
It would be good still to display the 'about' part of the page even if we cannot load committee info
client/src/pages/about.tsx
Outdated
| <div className="space-y-4 font-jersey10 text-base leading-relaxed text-white md:text-lg"> | ||
| <p> | ||
| Description of the clubs aims, why it exists, its mission, etc | ||
| etc. Second paragraph here, a second paragraph would be pretty | ||
| cool. The more info the better yippee!! | ||
| </p> | ||
| <p>Lorem ipsum dolor such and such I can't remember the rest.</p> |
There was a problem hiding this comment.
On the figma this paragraph is the sans font rather than jersey10 (similar to other body text around the site). If you prefer the look of jersey10 we could keep it, but if we want to be consistent with the rest of the site this should be the default sans.
Change Summary
Change Form
Other Information
There a definitely some small refinements that still need to be done, but other than that all the code is there. Notably there's probably some changes I could make to the error handling for the hook, and some of the frontend, but I don't know exactly and it would be great to get more feedback.
Related issue