Leadership Page Integration #63

2023-10-18T23:37:46Z

notsheesh commented

2023-10-18 23:37:46 +00:00

(Migrated from github.com)

Leadership Page Integration

Description

User Story

As a website visitor, I want to easily access information about the leaders and committees of SSEqual, so that I can learn more about the organization's team and structure.

Objective

This pull request addresses the integration of the "Leadership Page" into the main SSEqual website.
The goal is to seamlessly incorporate the leadership page while maintaining a consistent and cohesive user experience across the entire website.

Preview

https://github.com/rit-sse/WebsiteTheSSEquel/assets/39078738/da6116c4-c8e5-4ff2-bc61-ddf0d18cb6a0

# Leadership Page Integration # Description ## User Story As a website visitor, I want to easily access information about the leaders and committees of SSEqual, so that I can learn more about the organization's team and structure. ## Objective This pull request addresses the integration of the "Leadership Page" into the main SSEqual website. The goal is to seamlessly incorporate the leadership page while maintaining a consistent and cohesive user experience across the entire website. # Preview https://github.com/rit-sse/WebsiteTheSSEquel/assets/39078738/da6116c4-c8e5-4ff2-bc61-ddf0d18cb6a0

raynardmiot (Migrated from github.com) reviewed

2023-10-18 23:37:46 +00:00

2nPlusOne (Migrated from github.com) requested changes

2023-10-27 00:50:21 +00:00

2nPlusOne (Migrated from github.com) left a comment

Great work overall! I just have a few minor requests to clean up the file and utilize typescript more.

next/app/about/leadership/page.tsx

					
				@ -0,0 +1,61 @@

				import OfficerCard from "./OfficerCard";

Let's pull this component into a separate file, to clean up the code in this file. You could create an OfficerCard.tsx in the same folder alongside page.tsx, and export the component to be imported in the page.tsx file.

Let's pull this component into a separate file, to clean up the code in this file. You could create an `OfficerCard.tsx` in the same folder alongside page.tsx, and export the component to be imported in the page.tsx file.

I like that you have the 'content' pulled out into a data structure. Let's clean up this file some more by moving this array into a separate file in the same folder (a sibling to page.tsx)

Also, could you utilize typescript for each entry? You could create an 'officer.ts' file in the types folder, which defines all the fields. Then, you would use type annotations in your list declaration.

I like that you have the 'content' pulled out into a data structure. Let's clean up this file some more by moving this array into a separate file in the same folder (a sibling to page.tsx) Also, could you utilize typescript for each entry? You could create an 'officer.ts' file in the types folder, which defines all the fields. Then, you would use type annotations in your list declaration.

next/components/common/Icons.tsx

					
				@ -74,0 +83,4 @@

				/**

				 * Use stroke-* classes to change the border color of the icon.

				 * Use fill-* classes to change the fill color of the icon.

				 */

It looks like you replaced base-content with slate-400. To be clear, base-content refers to a color in the daisyui color theme that changes based on the data theme (dark or light mode). What was the reason for changing this back to slate-400? Maybe once I understand that, I can help figure out any color issues.

It looks like you replaced `base-content` with `slate-400`. To be clear, `base-content` refers to a color in the daisyui color theme that changes based on the data theme (dark or light mode). What was the reason for changing this back to slate-400? Maybe once I understand that, I can help figure out any color issues.

notsheesh (Migrated from github.com) reviewed

2023-10-27 01:29:19 +00:00

next/components/common/Icons.tsx

					
				@ -74,0 +83,4 @@

				/**

				 * Use stroke-* classes to change the border color of the icon.

				 * Use fill-* classes to change the fill color of the icon.

				 */

Oh, it was an uninformed decision. I have changed it back to base-content with room for customization.

Is this okay?

Icons.tsx

export const LinkedIn = ({color}) => (
    <svg 
        className={`w-5 h-5 fill-${color ? color : 'base-content'}`}
        viewBox="0 0 48 48"
    >
    // redacted for brevity
    </svg>
);

page.tsx

let iconColor = '';
<LinkedIn color={iconColor} />

Oh, it was an uninformed decision. I have changed it back to base-content with room for customization. Is this okay? ### Icons.tsx ``` export const LinkedIn = ({color}) => ( <svg className={`w-5 h-5 fill-${color ? color : 'base-content'}`} viewBox="0 0 48 48" > // redacted for brevity </svg> ); ``` ### page.tsx ``` let iconColor = ''; <LinkedIn color={iconColor} /> ```

2nPlusOne (Migrated from github.com) reviewed

2023-10-29 21:38:26 +00:00

next/components/common/Icons.tsx

					
				@ -74,0 +83,4 @@

				/**

				 * Use stroke-* classes to change the border color of the icon.

				 * Use fill-* classes to change the fill color of the icon.

				 */

I love that you're generalizing the icon's color! This is a great start, and it got me thinking of a few additional considerations.

What datatype should the component expect the "color" value to be passed in as? To US, it's obvious it's a string, but we should make it more clear. Typescript in the props can help with this.
Okay, so now we have types and it's clear it needs to be a string... What strings will be successfully parsed by the component?

As it stands, only strings for colors already defined by either tailwind or the daisyUI config will work. That is, "primary", "base-100", "emerald-400". It's fine to retain this limitation (because it means developers need to go out of their way to use a random color that might not match the palette), but documentation of this limitation/constraint would be beneficial to future developers and people using the component.

You could use JSdocs for this: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

I love that you're generalizing the icon's color! This is a great start, and it got me thinking of a few additional considerations. - What datatype should the component expect the "color" value to be passed in as? To US, it's obvious it's a string, but we should make it more clear. Typescript in the props can help with this. - Okay, so now we have types and it's clear it needs to be a string... What strings will be successfully parsed by the component? As it stands, only strings for colors already defined by either tailwind or the daisyUI config will work. That is, "primary", "base-100", "emerald-400". It's fine to retain this limitation (because it means developers need to go out of their way to use a random color that might not match the palette), but documentation of this limitation/constraint would be beneficial to future developers and people using the component. You could use JSdocs for this: https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

2nPlusOne (Migrated from github.com) reviewed

2023-10-29 21:58:33 +00:00

next/app/about/leadership/OfficerCard.tsx

					
				@ -0,0 +1,32 @@

				import { GitHub, LinkedIn, Email } from "../../../components/common/Icons";

The "gray-400" is a tailwind color, and won't change with the daisyUI data theme. Is there an alternative color from the daisyUI config color schemes that work well in dark/light modes?

next/app/about/leadership/team.ts

					
				@ -0,0 +1,127 @@

				export interface TeamMember {

You used the order in the existing website, which makes a lot of sense. The order in which we display the officers is something to think about, especially as the API is starting to be built out. Since this is all mock data, no need to spend time re-ordering it here.

The API endpoint to get all active officers could return the list in some default sort order, whether that be alphabetical by title or some other method. @joebaillie21

You used the order in the existing website, which makes a lot of sense. The order in which we display the officers is something to think about, especially as the API is starting to be built out. **Since this is all mock data, no need to spend time re-ordering it here.** The API endpoint to get all active officers could return the list in some default sort order, whether that be alphabetical by title or some other method. @joebaillie21

next/app/about/leadership/team.ts

					
				@ -0,0 +13,4 @@

				  committee_heads: TeamMember[];

				}

				export const teamData = {

export const teamData: Team = {

`export const teamData: Team = {`

Remember to use the types you defined 👍

2nPlusOne commented

2023-10-29 22:01:33 +00:00

(Migrated from github.com)

FYI, "draft" mode for pull requests usually means it isn't ready for final review, which isn't the case here.

2nPlusOne (Migrated from github.com) reviewed

2023-10-30 02:41:14 +00:00

next/components/common/Icons.tsx

					
				@ -74,0 +83,4 @@

				/**

				 * Use stroke-* classes to change the border color of the icon.

				 * Use fill-* classes to change the fill color of the icon.

				 */

Another option is to just have the whole className string passed in as a prop, which I think is the optimal solution as it lets us use the same icon svg with a lot more flexibility across the app.

2nPlusOne (Migrated from github.com) reviewed

2023-10-30 02:43:07 +00:00

next/app/about/leadership/OfficerCard.tsx

					
				@ -0,0 +1,32 @@

				import { GitHub, LinkedIn, Email } from "../../../components/common/Icons";

'neutral' should serve this purpose

2nPlusOne (Migrated from github.com) approved these changes

2023-11-12 20:17:17 +00:00

Sign in to join this conversation.

No reviewers

raynardmiot

2nPlusOne

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

SSE/WebsiteTheSSEquel!63

				`@ -0,0 +1,32 @@`
				`import { GitHub, LinkedIn, Email } from "../../../components/common/Icons";`

Rows
Columns