Redesign and update 'Meet the Team' data #452

Merged
j285he merged 17 commits from j285he-meet-the-team-s22 into main 2 months ago
j285he commented 3 months ago
Collaborator

Closes #450. Closes #436.

Closes #450. Closes #436.
j285he added 4 commits 3 months ago
j285he added 1 commit 3 months ago
5d2b718b23 Fix teammembercard
j285he changed title from Redesign and update 'Meet the Team' data to WIP: Redesign and update 'Meet the Team' data 3 months ago
j285he added 1 commit 3 months ago
425b05c6a6 Do not pass in metadata as props
j285he added 1 commit 3 months ago
789f57c0b3 Delete old images
j285he changed title from WIP: Redesign and update 'Meet the Team' data to Redesign and update 'Meet the Team' data 3 months ago
j285he changed title from Redesign and update 'Meet the Team' data to WIP: Redesign and update 'Meet the Team' data 3 months ago
j285he added 2 commits 3 months ago
ad224d3eed Add new members
j285he changed title from WIP: Redesign and update 'Meet the Team' data to Redesign and update 'Meet the Team' data 3 months ago
Poster
Collaborator
There is no content yet.
174 KiB
259 KiB
177 KiB
133 KiB
206 KiB
Poster
Collaborator
There is no content yet.
158 KiB
j285he requested review from n3parikh 3 months ago
j285he requested review from a258wang 3 months ago
a3thakra reviewed 3 months ago
import React from "react";
import { Metadata } from "@/lib/team";
Owner

Components should be isolated and self contained. They should not be importing things from outside the /components folder.

Components should be isolated and self contained. They should not be importing things from outside the /components folder.
Owner

This is why we duplicated an interface here to TeamMemberProps. This makes the component independent of whatever the Metadata is. If in the future the metadata changes, the props of this component would also change (or the other way around) - which we may not want.

This is why we duplicated an interface here to `TeamMemberProps`. This makes the component independent of whatever the Metadata is. If in the future the metadata changes, the props of this component would also change (or the other way around) - which we may not want.
j285he marked this conversation as resolved
a3thakra reviewed 3 months ago
photography = sortTeam(photography);
website = sortTeam(website);
systems = sortTeam(systems);
terminal = sortTeam(terminal);
Owner

This is getting a little out of hand. With these many teams, can you move them into a 2d array?

This is getting a little out of hand. With these many teams, can you move them into a 2d array?
a3thakra reviewed 3 months ago
return {
props: {
execs,
coordinators,
Owner

this way you can make the props: execs (a 1d array) and teams: (a 2d array)

this way you can make the props: execs (a 1d array) and teams: (a 2d array)
Poster
Collaborator

What about the teams array on line 64? Won't we need to change each members property to be (for example) members: teams[0]?

What about the `teams` array on line 64? Won't we need to change each `members` property to be (for example) `members: teams[0]`?
Poster
Collaborator

I was also thinking perhaps teams could go in a contents .json file but let me know what you think is best

I was also thinking perhaps `teams` could go in a contents .json file but let me know what you think is best
Owner

Yeah if we move it into an array here, then we won't need the array on line 64, and a lot of hardcoding would go away.

Perhaps putting all the teams in one place would be a better idea.

Eg - instead of /contents/team/team-name.json we can put all the json data in an array in /contents/team/non-execs.json which could follow the following format:

[
  {
    id: "website",
    name: "Web Committee",
    members: [
      {
        ...
      },
      ...
    ]
  },
  ...
]

But don't block this PR on this change. Let's get the content updated first and you can file an issue to get back to this :)

Yeah if we move it into an array here, then we won't need the array on line 64, and a lot of hardcoding would go away. Perhaps putting all the teams in one place would be a better idea. Eg - instead of `/contents/team/team-name.json` we can put all the json data in an array in `/contents/team/non-execs.json` which could follow the following format: ``` [ { id: "website", name: "Web Committee", members: [ { ... }, ... ] }, ... ] ``` But don't block this PR on this change. Let's get the content updated first and you can file an issue to get back to this :)
a3thakra reviewed 3 months ago
const teamMembers: Metadata[] = [];
const teamOthers: Metadata[] = [];
for (const member of team) {
if (!Object.prototype.hasOwnProperty.call(member, "role")) {
Owner

if (member.hasOwnProperty("role")) {

`if (member.hasOwnProperty("role")) {`
Owner

or if (member["role"] != null)

or `if (member["role"] != null)`
j285he marked this conversation as resolved
a3thakra reviewed 3 months ago
}
teamLeads.sort((a, b) => a.name.localeCompare(b.name));
teamMembers.sort((a, b) => a.name.localeCompare(b.name));
teamOthers.sort((a, b) => a.name.localeCompare(b.name));
Owner

nit: Instead of using that for loop, I would suggest using a more declarative approach.

const leads = team.filter(({role}) => role === "Team Lead").sort(memberComparer);
const general = team.filter(({role}) => role == null || role === "").sort(memberComparer);
const others = team.filter(({role}) => role != null && role !== "" && role !== "Team Lead").sort(memberComparer);

return [...leads, ...general, ...others]

function memberComparer(a: Metadata, b: Metadata) {
	return a.name.localeCompare(b.name)
}
nit: Instead of using that for loop, I would suggest using a more declarative approach. ```typescript const leads = team.filter(({role}) => role === "Team Lead").sort(memberComparer); const general = team.filter(({role}) => role == null || role === "").sort(memberComparer); const others = team.filter(({role}) => role != null && role !== "" && role !== "Team Lead").sort(memberComparer); return [...leads, ...general, ...others] function memberComparer(a: Metadata, b: Metadata) { return a.name.localeCompare(b.name) } ```
j285he marked this conversation as resolved
Owner

yay all the old pictures are gone!

Left comments for code nits, but looks good overall :) Thanks @j285he!!

yay all the old pictures are gone! Left comments for code nits, but looks good overall :) Thanks @j285he!!
a258wang reviewed 2 months ago
"name": "Andrew Wang"
},
{
"name": "Anthony Brennan",
Owner

@j285he I suspect we want to keep all these alumni-labelled peeps, but let's let @r389li confirm that. Sorry for not being more clear with expectations 😅

@j285he I suspect we want to keep all these alumni-labelled peeps, but let's let @r389li confirm that. Sorry for not being more clear with expectations 😅
j285he marked this conversation as resolved
Owner

@a258wang Correct, we should keep them. Syscom membership doesn't usually change. This term, however, Max graduated, so he is now Alumni, and I misidentified Edwin as alumni when he's in fact a current student. Also if someone could link syscom and termcom in my bio to the relevant sections on the page that would be appreciated!

@a258wang Correct, we should keep them. Syscom membership doesn't usually change. This term, however, Max graduated, so he is now Alumni, and I misidentified Edwin as alumni when he's in fact a current student. Also if someone could link syscom and termcom in my bio to the relevant sections on the page that would be appreciated!
j285he added 3 commits 2 months ago
j285he added 1 commit 2 months ago
ddf6ac4b9b use declarative apporach
j285he added 2 commits 2 months ago
a258wang reviewed 2 months ago
.filter(({ role }) => role === "Team Lead")
.sort(memberComparer);
const general = team
.filter(({ role }) => role == null || role === "")
Owner

Would role == null || role === "" be equivalent to !role here? (and also a couple lines below)

Would `role == null || role === ""` be equivalent to `!role` here? (and also a couple lines below)
j285he marked this conversation as resolved
a258wang approved these changes 2 months ago
a258wang left a comment
Owner

Just one tiny (non-blocking) nitpick, but otherwise this looks great! 🚢

Once this is merged, could you please create a ticket to clean up how we store/process the team data, as discussed in the comments of this PR? Thanks!

Just one tiny (non-blocking) nitpick, but otherwise this looks great! 🚢 Once this is merged, could you please create a ticket to clean up how we store/process the team data, as discussed in the comments of this PR? Thanks!
j285he added 2 commits 2 months ago
65eaaa12f0 Change !role
j285he merged commit e1af564621 into main 2 months ago
j285he deleted branch j285he-meet-the-team-s22 2 months ago

Reviewers

n3parikh was requested for review 3 months ago
a258wang approved these changes 2 months ago
continuous-integration/drone/push Build is passing
The pull request has been merged as e1af564621.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

This pull request currently doesn't have any dependencies.

Loading…
There is no content yet.