Add officer card data and semester data #35
Conversation
…mage and linkedin.
…issing Officer photos with ssd logo due to error in loading image or not having the image
There was a problem hiding this comment.
Requesting changes for a few things in the rendering logic
Looking at officer data, I'm noticing some quirks here with how this is designed. In the interest of time and drawing this to a close I'm going to open a PR off of this one to make improvements here and then we can merge that into this PR. That'll give you a chance to review it and highlight differences rather than correcting things here.
|
|
||
| export default function About() { | ||
| const officerCards = officerCardData.map((cardData) => ( |
There was a problem hiding this comment.
[Nit]: Development comments like this should be cleaned up before merging under most situations.
src/app/about/page.tsx
Outdated
| ([semester, officers]) => { | ||
| const officerCards = Object.entries(officers).map( | ||
| ([position, officer]) => { | ||
| if (officer === undefined) { |
There was a problem hiding this comment.
[Please Change]: From our discussion the handling here has been changed and this check is no longer needed.
src/app/about/page.tsx
Outdated
|
|
||
| return ( | ||
| <OfficerCard | ||
| key={`${semester} ${position}`} |
There was a problem hiding this comment.
[Please Change]: We can use just position as the key here because it's unique within each semester. In other words; we don't need to ensure the keys are unique across both loops; just the current one.
| ); | ||
|
|
||
| return ( | ||
| <React.Fragment key={semester}> |
There was a problem hiding this comment.
[Comment]: Great use of React.Fragment here - definitely how I would have gone about this to avoid some unnecessary div wrapping.
src/app/about/page.tsx
Outdated
| @@ -41,7 +71,8 @@ export default function About() { | |||
| <section className={styles.officers}> | |||
| <div className={styles.container}> | |||
| <h2>Officers</h2> | |||
| <div className={styles.cardContainer}>{officerCards}</div> | |||
| {/* <div className={styles.cardContainer}>{officerCards}</div> */} | |||
There was a problem hiding this comment.
[Nit]: Can be cleaned up as above.
src/app/about/officer-data.ts
Outdated
| @@ -26,7 +31,7 @@ const officerCardData: OfficerCardProps[] = [ | |||
| ], | |||
| }, | |||
| { | |||
| position: "Treasurer", | |||
| position: "Tresurer", | |||
There was a problem hiding this comment.
[Comment]: Some of the changes in here have spelling mistakes; also noticing the officer images added in this specific commit are invalid and can't be displayed - maybe this commit wasn't intended to be included?
See review summary notes - will address issues related to officer data separately.
Added officer card and semester objects with Brian's help.