feat(frontend): add reusable executive info cards#68
Conversation
ashoomky
left a comment
ashoomky
left a comment
There was a problem hiding this comment.
Great work Hayden! Just some minor things to fix up:
Two font fixes needed: both the role label and name <h3> use font-heading (Staatliches) but Figma specifies Inter, so make sure to swap both to font-body with the correct weight. While you're on the role label, change break-all to break-words to fix the mid-word splitting.
Also either add a bio prop and render it, or remove the field from the schema, as right now it collects data that goes nowhere. The string branch in the photo type can also be dropped since depth is always ≥ 1 on fetch.
For the executives/page.tsx, delete the ExecutiveCardData type and import Executive from @/payload-types instead, as it's already generated from the schema and won't drift when fields change.
Lastly, for proper conventions make sure to create a ExecCard folder, and put the ExecCard.tsx file in that folder. Refer to Navbar for example.
| type ExecutiveCardData = { | ||
| id: string | ||
| role: string | ||
| name: string | ||
| degree?: string | null | ||
| position?: string | null | ||
| yearsOfExperience?: number | null | ||
| photo?: | ||
| | { | ||
| url?: string | null | ||
| alt?: string | null | ||
| } | ||
| | string | ||
| | null | ||
| } |
There was a problem hiding this comment.
This type already exists as Executive in payload-types.ts. Payload generates it automatically from the collection schema. If a field is added or changed in the schema, payload-types.ts updates but this type will silently drift. Delete this block and import Executive from @/payload-types instead.
| limit: 100, | ||
| }) | ||
|
|
||
| const executives = result.docs as ExecutiveCardData[] |
There was a problem hiding this comment.
Change to result.docs as Executive[] once the manual type above is removed
| photo?: | ||
| | { | ||
| url?: string | null | ||
| alt?: string | null | ||
| } | ||
| | string | ||
| | null | ||
| } |
There was a problem hiding this comment.
The string branch here will never be hit in practice. Payload always returns a populated media object or null for an upload field, never a raw URL string. The union adds complexity without benefit, so to fix this simplify to { url?: string | null; alt?: string | null } | null.
|
|
||
| return ( | ||
| <article className="mx-auto flex w-full max-w-[220px] flex-col items-center text-center"> | ||
| <p className="mb-3 max-w-full break-all text-center font-heading text-[#7FA6D8] text-sm uppercase tracking-wide sm:text-base"> |
There was a problem hiding this comment.
Two issues here: break-all splits at any character boundary which causes the ugly mid-word wrapping seen in testing (where you had BBBBBBB). Change to break-words.
Also the font for people's names and their roles, should be font Inter as per Figma, right now it's Staatliches. Fix this please!
| )} | ||
| </div> | ||
|
|
||
| <h3 className="mb-2 font-heading text-brand-primary text-lg uppercase leading-tight sm:text-xl"> |
There was a problem hiding this comment.
Same font issue as the role label, font-heading resolves to Staatliches but Figma uses Inter here. Change to font-body font-bold (or whichever weight the spec calls for).
| {degree ? ( | ||
| <p className="font-body text-brand-primary text-sm leading-snug">{degree}</p> | ||
| ) : null} | ||
| {position ? ( | ||
| <p className="font-body text-brand-navy text-sm leading-snug">{position}</p> | ||
| ) : null} | ||
| {typeof yearsOfExperience === "number" ? ( | ||
| <p className="font-body text-brand-navy text-sm leading-snug"> | ||
| {yearsOfExperience} years exp. | ||
| </p> | ||
| ) : null} |
There was a problem hiding this comment.
The Executives collection defines a bio field but it's not in ExecCardProps and isn't rendered here, so anything entered in the CMS for bio is silently discarded. Either add a bio?: string | null prop and render it, or remove the field from the collection config.
ashoomky
left a comment
ashoomky
left a comment
There was a problem hiding this comment.
Looking good! Just a few minor tweaks before the final merge:
Two changes still needed - The executives page currently renders all cards in a flat grid, change it so that it groups executives by role category and render a section header before each group (e.g. "CO-PRESIDENTS", "ADMIN TEAM"). Also, the in ExecCard is missing a sizes="220px" prop - without it Next.js defaults to 100vw and fetches a full-viewport-width image for what is always a 220px card.
| <main className="min-h-screen bg-brand-light-grey px-6 py-10 sm:px-8 sm:py-12"> | ||
| <div className="mx-auto max-w-4xl"> | ||
| <h1 className="mb-10 text-center font-heading text-3xl text-brand-primary uppercase sm:text-4xl"> | ||
| Executive Team | ||
| </h1> | ||
|
|
||
| {executives.length === 0 ? ( | ||
| <p className="text-center font-body text-brand-primary text-lg"> | ||
| No executive profiles to display. | ||
| </p> | ||
| ) : ( | ||
| <div className="grid justify-items-center gap-x-6 gap-y-10 sm:grid-cols-2 lg:grid-cols-3"> | ||
| {executives.map((executive) => ( | ||
| <ExecCard | ||
| degree={executive.degree} | ||
| key={executive.id} | ||
| name={executive.name} | ||
| photo={executive.photo} | ||
| photo={ | ||
| executive.photo && typeof executive.photo !== "string" | ||
| ? { | ||
| url: executive.photo.url, | ||
| alt: executive.photo.alt, | ||
| } | ||
| : null | ||
| } | ||
| position={executive.position} | ||
| role={executive.role} | ||
| yearsOfExperience={executive.yearsOfExperience} | ||
| /> | ||
| ))} | ||
| </div> |
There was a problem hiding this comment.
Grouping needed:
The page renders all executives in a single flat grid. But the page needs to show executives visually grouped under section headers ("CO-PRESIDENTS", "ADMIN TEAM", etc.). The grouping logic is currently absent so you'd need to change it to group executives by role category and render a header per group before the cards.
| <div className="relative mb-8 h-[220px] w-[220px] overflow-hidden bg-[#d9d9d9]"> | ||
| <div className="relative mb-8 h-[220px] w-[220px] overflow-hidden bg-[#D9D9D9]"> | ||
| {photoUrl ? ( | ||
| <Image alt={photoAlt} className="object-cover" fill src={photoUrl} /> |
There was a problem hiding this comment.
In Next.js, Image needs a sizes hint to pick the right source size. Without it, it defaults to 100vw and will fetch a full-viewport-width image for what is always a 220px card.
Should be:
<Image alt={photoAlt} className="object-cover" fill sizes="220px" src={photoUrl} />
2 participants
Description
Implements a reusable executive member info card for the About page.
This PR:
ExecCardcomponentdegree,position,yearsOfExperience, andphoto)/executivespage for testingCloses #59
Type of change
How Has This Been Tested?
Tested:
Checklist before requesting a review