Feature: Officer Section #21
Conversation
Co-authored-by: Phantom0110 <146048575+phantom0110@users.noreply.github.com>
|
Here is what the current card looks like. Note that we had to make the card with a dark background to make the icons show up properly. |
Co-authored-by: Phantom0110 <146048575+Phantom0110@users.noreply.github.com>
|
This is how multiple cards will render on the page. We noticed an issue with the background; the gradient resets after a certain number of pixels, making this weird edge appear. |
|
Card looks good. The gradient reset with the background was fixed in #26; make sure to rebase and address the conflicts. |
…o add officer pictures and social links.
…o seem to not fit well in the officer card and social media icons popping out of card as a result. #18
How cards currently look.
|
…feature/officer-section
|
|
| The website logo is slightly smaller than the other logos. | ||
| This scales it up a tiny bit. | ||
| This is where the logo comes from: | ||
| https://fonts.google.com/icons?selected=Material%20Symbols%20Outlined%3Alanguage%3AFILL%401%3Bwght%40400%3BGRAD%400%3Bopsz%4024 |
There was a problem hiding this comment.
[Comment]: Good callout & use of comments. May be worth looking into later, but let's accept this limitation for now. I'll sync with you later and see if we should open a backlog ticket to dig into this more.
| image={cardData.image} | ||
| alt={cardData.alt} | ||
| name={cardData.name} | ||
| socialLinksData={cardData.socialLinksData} |
There was a problem hiding this comment.
[Suggestion]: You can use {...cardData} to spread props into the component, save a bit of work/duplication here.
There was a problem hiding this comment.
Can't change this due to eslint react/jsx-props-no-spreading. I've decided to keep it as is rather than disable the rule. Let me know if you insist this be changed.
There was a problem hiding this comment.
That's fine, no strong opinion here. I'd guess that the use of TypeScript also helps catch many of the common bugs here.
| /> | ||
| </div> | ||
| <section className={styles.officers}> | ||
| <div className={styles.container}> |
There was a problem hiding this comment.
[Comment]: Will probably have to abstract this down the line when we add past officers. Don't change anything here; just making note and musing for the future.
src/app/about/officer-card.tsx
Outdated
| export type OfficerCardProperties = { | ||
| position: string; | ||
| image: string; | ||
| alt: string; |
There was a problem hiding this comment.
[Comment/Suggestion]: It's unusual to see an alt text property for a generic component like this (especially given it's set to an empty string when used below). I would suggest computing something like Profile picture of ${name}. instead, I think that's good enough for our needs.
src/app/about/page.tsx
Outdated
| @@ -1,7 +1,111 @@ | |||
| import Image from "next/image"; | |||
| import styles from "./About.module.css"; | |||
|
|
|||
| import OfficerCard, { OfficerCardProperties } from "./officer-card"; | |||
|
|
|||
| const officerCardData: OfficerCardProperties[] = [ | |||
There was a problem hiding this comment.
[Comment]: This may be a good candidate to have in a separate file, that way it can just be referenced by this page. I'd expect this constant to get a lot messier once we start adding previous officers.
There was a problem hiding this comment.
I agree. I like to put data like this in separate files. In previous projects, I've stored data in .ts files in a separate data/ directory. We could do that too, or put data in the src/app/about/ directory. I'd like to know what you think (and if this should maybe be saved for a separate issue).
There was a problem hiding this comment.
I'm generally inclined to always abstract by usage - in other words, a simple officers.ts file (or officer-data.ts, if preferred) in the about directory is probably the best option for now. I'm not particularly a fan of diving projects up into categories like /components, /forms, /types, etc. I think it forces more abstraction than is often needed (or conceptually present) and grouping by programmatic concern is less valuable than grouping by business concern.
| image={cardData.image} | ||
| alt={cardData.alt} | ||
| name={cardData.name} | ||
| socialLinksData={cardData.socialLinksData} |
There was a problem hiding this comment.
That's fine, no strong opinion here. I'd guess that the use of TypeScript also helps catch many of the common bugs here.
Resolves #18. Adds officer cards to About page to display positions, names, and social links. Should be sufficiently responsive. Links connect to real web pages. I've added LinkedIn pages for everyone, and also GitHub and Website Links for Angel since I was able to find them and they seemed fitting. The number of columns changes with screen width; I believe this should be sufficiently responsive for mobile views (see below).