Meet the Team page #94

Merged
b38peng merged 28 commits from feat/meet-the-team-page into main 2021-08-23 11:11:47 -04:00
Member

Includes mobile Team Member Card

Closes #9
Closes #42

Includes mobile Team Member Card Closes #9 Closes #42
b38peng added 15 commits 2021-07-24 20:59:28 -04:00
b38peng requested review from a3thakra 2021-07-24 21:00:02 -04:00
b38peng requested review from n3parikh 2021-07-24 21:00:05 -04:00
b38peng added 1 commit 2021-07-25 11:23:22 -04:00
continuous-integration/drone/push Build is passing Details
9964e54d69
our supporters margin fix
n3parikh reviewed 2021-08-01 18:03:31 -04:00
@ -0,0 +1,7 @@
---
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 2021-08-01 18:07:36 -04:00
@ -55,1 +55,3 @@
@media only screen and (max-width: calc(375rem / 16)) {
/* Popup */
.popup_background {
Owner

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 2021-08-01 18:09:48 -04:00
@ -0,0 +72,4 @@
<Bubble>
<div className={styles.elections}>
<h2 className={styles.election_subheading}>Elections</h2>
<ElectionContent />
Owner

You can inline this

You can inline this
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-01 18:12:01 -04:00
@ -0,0 +36,4 @@
return windowSize;
}
export default useWindowDimension;
Owner

avoid default exports as that prevents auto imports

avoid default exports as that prevents auto imports
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-01 18:14:34 -04:00
lib/team.ts Outdated
@ -0,0 +18,4 @@
.map((name) => name.slice(0, -1 * fileType.length));
}
export async function getExecContent(fileName: string, convert = true) {
Owner

getExec is fine

getExec is fine
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-01 18:16:29 -04:00
@ -0,0 +1,39 @@
import { useEffect, useState } from "react";
Owner

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`
Author
Member

both lint and lint:fix right?

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

yep!

yep!
a3thakra marked this conversation as resolved
Owner

Font size for non-execs is too small

Font size for non-execs is too small
Owner

There's a lot of space in the bottom

There's a lot of space in the bottom
a3thakra reviewed 2021-08-01 18:35:58 -04:00
@ -0,0 +25,4 @@
execs: SerializedExec[];
}
export default function MeetTheTeam({ execs }: Props) {
Owner

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
Owner

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.
Author
Member

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 2021-08-05 17:18:51 -04:00
Owner

@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 2021-08-08 20:15:35 -04:00
@ -0,0 +1,7 @@
---
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 2021-08-08 22:22:50 -04:00
b38peng added 1 commit 2021-08-09 18:45:17 -04:00
continuous-integration/drone/push Build is failing Details
972b7f4200
dynamic image urls + animation
a3thakra reviewed 2021-08-09 19:21:00 -04:00
@ -0,0 +103,4 @@
);
}
async function getMemberImage(team: Omit<Metadata, "image">[]) {
Owner

getTeamWithImages

`getTeamWithImages`
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-09 19:24:12 -04:00
@ -0,0 +122,4 @@
)) as SerializedExec[];
const programme = await getMemberImage(programmeData);
const website = await getMemberImage(websiteData);
const systems = await getMemberImage(systemsData);
Owner

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 2021-08-09 20:03:21 -04:00
@ -21,0 +37,4 @@
<h2 className={styles.role}>{role}</h2>
<div className={styles.description}>{children}</div>
</article>
{isOpen && (
Owner

isOpen && width <= 768

isOpen && width <= 768
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-09 20:06:09 -04:00
@ -78,0 +92,4 @@
padding: calc(20rem / 16) calc(40rem / 16);
left: 0;
top: 50%;
animation: popup 0.7s forwards;
Owner

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
Author
Member

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 2021-08-13 05:10:10 -04:00
@ -0,0 +50,4 @@
}
.elections {
margin-top: 6rem;
Owner

Looks like it's coming from here

Looks like it's coming from here
b38peng marked this conversation as resolved
a3thakra reviewed 2021-08-13 05:13:32 -04:00
@ -0,0 +53,4 @@
margin-top: 6rem;
}
.electionSubheading {
Owner

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.
Author
Member

oH my bad thanks!!

oH my bad thanks!!
b38peng marked this conversation as resolved
b38peng added 2 commits 2021-08-18 21:35:54 -04:00
b38peng added 1 commit 2021-08-18 21:43:27 -04:00
continuous-integration/drone/push Build is passing Details
29477ccb4b
lint fix
a3thakra reviewed 2021-08-19 00:25:25 -04:00
@ -0,0 +21,4 @@
import styles from "./team.module.css";
// TODO: link News page in Elections
Owner

Link to homepage

Link to homepage
Owner

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
Owner

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
Owner

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

Looks good apart from that one bug. Great job @b38peng!
b38peng added 1 commit 2021-08-20 19:40:38 -04:00
continuous-integration/drone/push Build is passing Details
1b0655c0c4
fix
b38peng added 1 commit 2021-08-20 19:41:45 -04:00
a3thakra approved these changes 2021-08-22 05:40:40 -04:00
a3thakra left a comment
Owner
HIT THAT BUTTON YAY https://media.giphy.com/media/vMNoKKznOrUJi/giphy.gif
Owner

Ugh 🤦

Ugh 🤦 ![](https://media.giphy.com/media/vMNoKKznOrUJi/giphy.gif)
b38peng added 1 commit 2021-08-23 10:44:09 -04:00
b38peng merged commit a8cae99c11 into main 2021-08-23 11:11:47 -04:00
b38peng referenced this issue from a commit 2021-08-23 11:11:48 -04:00
b38peng deleted branch feat/meet-the-team-page 2021-08-23 11:12:16 -04:00
Sign in to join this conversation.
No reviewers
No Milestone
No project
No Assignees
3 Participants
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: www/www-new#94
No description provided.