Meet the Team page #94

Merged
b38peng merged 28 commits from feat/meet-the-team-page into main 1 year ago
b38peng commented 1 year ago
Collaborator

Includes mobile Team Member Card

Closes #9
Closes #42

Includes mobile Team Member Card Closes #9 Closes #42
b38peng added 15 commits 1 year ago
b38peng requested review from a3thakra 1 year ago
b38peng requested review from n3parikh 1 year ago
b38peng added 1 commit 1 year ago
9964e54d69 our supporters margin fix
n3parikh reviewed 1 year ago
---
name: Gordon Le
role: Vice President
image: /images/team/exec/Exec_GordonLe.jpg
Owner

Non-Mac OS filesystems are case sensitive, so this will 404, since the file ends with a JPG, not jpg. There's a few other images that have the same problem, could you fix that up? Thanks!

Non-Mac OS filesystems are case sensitive, so this will 404, since the file ends with a `JPG`, not `jpg`. There's a few other images that have the same problem, could you fix that up? Thanks!
a3thakra marked this conversation as resolved
a3thakra reviewed 1 year ago
@media only screen and (max-width: calc(375rem / 16)) {
/* Popup */
.popup_background {
Collaborator

We are using camelCase class names in css, and not snake_case

We are using camelCase class names in css, and not snake_case
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
<Bubble>
<div className={styles.elections}>
<h2 className={styles.election_subheading}>Elections</h2>
<ElectionContent />
Collaborator

You can inline this

You can inline this
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
return windowSize;
}
export default useWindowDimension;
Collaborator

avoid default exports as that prevents auto imports

avoid default exports as that prevents auto imports
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
lib/team.ts Outdated
.map((name) => name.slice(0, -1 * fileType.length));
}
export async function getExecContent(fileName: string, convert = true) {
Collaborator

getExec is fine

getExec is fine
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
import { useEffect, useState } from "react";
Collaborator

Since you created a new folder, you should also add it to the lint command in package.json

Since you created a new folder, you should also add it to the `lint` command in `package.json`
Poster
Collaborator

both lint and lint:fix right?

both `lint` and `lint:fix` right?
Collaborator

yep!

yep!
a3thakra marked this conversation as resolved
Collaborator

Font size for non-execs is too small

Font size for non-execs is too small
Collaborator

There's a lot of space in the bottom

There's a lot of space in the bottom
a3thakra reviewed 1 year ago
execs: SerializedExec[];
}
export default function MeetTheTeam({ execs }: Props) {
Collaborator

nit-pick: This should match the name of the file, so function name should be Team

nit-pick: This should match the name of the file, so function name should be `Team`
b38peng marked this conversation as resolved
Collaborator

I'm wondering if we should dynamically create image paths, instead of hardcoding image paths 🤔 @n3parikh thoughts?

I'm wondering if we should dynamically create image paths, instead of hardcoding image paths 🤔 @n3parikh thoughts?
Owner

One change I think would be worth making would be to drop the exec/systems/programme/website folder, and then also get rid of the team prefix on each image. I don't think those provide much use, and if people are on multiple teams, or change teams, it's more work to handle.

So the end strcture would be public/images/team/FirstLast.jpg.

Dynamic image paths sounds good to me. Doing the above change would make it a bit easier too.

One change I think would be worth making would be to drop the `exec`/`systems`/`programme`/`website` folder, and then also get rid of the team prefix on each image. I don't think those provide much use, and if people are on multiple teams, or change teams, it's more work to handle. So the end strcture would be `public/images/team/FirstLast.jpg`. Dynamic image paths sounds good to me. Doing the above change would make it a bit easier too.
Poster
Collaborator

One change I think would be worth making would be to drop the exec/systems/programme/website folder, and then also get rid of the team prefix on each image. I don't think those provide much use, and if people are on multiple teams, or change teams, it's more work to handle.

So the end strcture would be public/images/team/FirstLast.jpg.

Dynamic image paths sounds good to me. Doing the above change would make it a bit easier too.

If we dynamically imported img paths, how should I create the array of members in the .json files? If we mapped through the img files, the name field we can prolly extrapolate from the file name, but what about the role? @n3parikh @a3thakra

> One change I think would be worth making would be to drop the `exec`/`systems`/`programme`/`website` folder, and then also get rid of the team prefix on each image. I don't think those provide much use, and if people are on multiple teams, or change teams, it's more work to handle. > > So the end strcture would be `public/images/team/FirstLast.jpg`. > > Dynamic image paths sounds good to me. Doing the above change would make it a bit easier too. If we dynamically imported img paths, how should I create the array of members in the `.json` files? If we mapped through the img files, the `name` field we can prolly extrapolate from the file name, but what about the `role`? @n3parikh @a3thakra
b38peng added 2 commits 1 year ago
Collaborator

@b38peng the actual json files or .md files will not have an image field. But you will be writing a function inside the lib/team file that finds an image for a person with some name. You can then use this function for dynamically generating the "image" field for the metadata.

The nodejs fs API docs should be helpful. https://nodejs.org/api/fs.html

@b38peng the actual json files or .md files will not have an `image` field. But you will be writing a function inside the `lib/team` file that finds an image for a person with some name. You can then use this function for dynamically generating the "image" field for the metadata. The nodejs fs API docs should be helpful. https://nodejs.org/api/fs.html
n3parikh reviewed 1 year ago
---
name: Gordon Le
role: Vice President
image: /images/team/exec/Exec_GordonLe.JPG
Owner

Sorry, I should have been more clear, can you change the file exstenstion instead, and just make everything lowercase, for consistency. It'll make the code to generate the URL simpler as well.

Sorry, I should have been more clear, can you change the file exstenstion instead, and just make everything lowercase, for consistency. It'll make the code to generate the URL simpler as well.
b38peng marked this conversation as resolved
b38peng added 3 commits 1 year ago
b38peng added 1 commit 1 year ago
972b7f4200 dynamic image urls + animation
a3thakra reviewed 1 year ago
);
}
async function getMemberImage(team: Omit<Metadata, "image">[]) {
Collaborator

getTeamWithImages

`getTeamWithImages`
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
)) as SerializedExec[];
const programme = await getMemberImage(programmeData);
const website = await getMemberImage(websiteData);
const systems = await getMemberImage(systemsData);
Collaborator

We can do all of this at the same time, no need to wait for waiting for one team before proceeding to the next.

const [programme, website, systems] = await Promise.all([
  getTeamWithImages(programmeData),
  getTeamWithImages(websiteData),
  getTeamWithImages(systemsData),
])
We can do all of this at the same time, no need to wait for waiting for one team before proceeding to the next. ```ts const [programme, website, systems] = await Promise.all([ getTeamWithImages(programmeData), getTeamWithImages(websiteData), getTeamWithImages(systemsData), ]) ```
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
<h2 className={styles.role}>{role}</h2>
<div className={styles.description}>{children}</div>
</article>
{isOpen && (
Collaborator

isOpen && width <= 768

isOpen && width <= 768
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
padding: calc(20rem / 16) calc(40rem / 16);
left: 0;
top: 50%;
animation: popup 0.7s forwards;
Collaborator

rn, there's no animation when we're closing the popup. We can fix that by always rendering the popup, but hiding it on desktop screens, and only toggling a class on mobiles.

rn, there's no animation when we're closing the popup. We can fix that by always rendering the popup, but hiding it on desktop screens, and only toggling a class on mobiles.
b38peng marked this conversation as resolved
Poster
Collaborator

There's a lot of space in the bottom

This seems to be from the Bubble component :o @a3thakra

> There's a lot of space in the bottom This seems to be from the Bubble component :o @a3thakra
a3thakra reviewed 1 year ago
}
.elections {
margin-top: 6rem;
Collaborator

Looks like it's coming from here

Looks like it's coming from here
b38peng marked this conversation as resolved
a3thakra reviewed 1 year ago
margin-top: 6rem;
}
.electionSubheading {
Collaborator

Also here, h2 have a default margin-top of 32px, so you would need to make that 0 here.

Also here, `h2` have a default margin-top of 32px, so you would need to make that 0 here.
Poster
Collaborator

oH my bad thanks!!

oH my bad thanks!!
b38peng marked this conversation as resolved
b38peng added 2 commits 1 year ago
b38peng added 1 commit 1 year ago
29477ccb4b lint fix
a3thakra reviewed 1 year ago
import styles from "./team.module.css";
// TODO: link News page in Elections
Collaborator

Link to homepage

Link to homepage
Collaborator

maybe add an id to the news container, and make that a part of the link.

maybe add an id to the news container, and make that a part of the link.
b38peng marked this conversation as resolved
Collaborator

On an iPhone X:

  • In portrait mode, open up a popup
  • Now switch to landscape mode
  • I can't close the popup
On an iPhone X: - In portrait mode, open up a popup - Now switch to landscape mode - I can't close the popup
Collaborator

Looks good apart from that one bug. Great job @b38peng!

Looks good apart from that one bug. Great job @b38peng!
b38peng added 1 commit 1 year ago
1b0655c0c4 fix
b38peng added 1 commit 1 year ago
a3thakra approved these changes 1 year ago
a3thakra left a comment
Collaborator
HIT THAT BUTTON YAY https://media.giphy.com/media/vMNoKKznOrUJi/giphy.gif
Collaborator

Ugh 🤦

Ugh 🤦 ![](https://media.giphy.com/media/vMNoKKznOrUJi/giphy.gif)
b38peng added 1 commit 1 year ago
b38peng merged commit a8cae99c11 into main 1 year ago
b38peng referenced this issue from a commit 1 year ago
b38peng deleted branch feat/meet-the-team-page 1 year ago

Reviewers

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

No due date set.

Dependencies

No dependencies set.

Reference: www/www-new#94
Loading…
There is no content yet.