-
Notifications
You must be signed in to change notification settings - Fork 1
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
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.
LGTM following #37!
Added officer card and semester objects with Brian's help.