-
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
Feature: Officer Section #21
Conversation
Co-authored-by: Phantom0110 <146048575+phantom0110@users.noreply.github.com>
Co-authored-by: Phantom0110 <146048575+Phantom0110@users.noreply.github.com>
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
…feature/officer-section
|
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.
Minor changes requested (pardon the delay!), and a few larger suggestion/comments on things. The card itself looks good and handles column responsiveness well too.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).