Skip to content
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

Organizer page is now fetching from Sanity #21

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

waalbert
Copy link

A few notes

  • The organizer page doesn't show anyone's pronouns (not sure where exactly they would go - probably after or under the name)
  • Instead of saying "Tech Organizers" like the current hack.ics.uci.edu site says, this site says "tech." This could be changed in a future UI fix. Same goes for the other departments.
    image

@waalbert waalbert linked an issue Jul 28, 2023 that may be closed by this pull request
@github-actions
Copy link

github-actions bot commented Jul 28, 2023

Deploy preview for hack-ics-uci-edu ready!

Name Hack at UCI Site
Preview Visit Preview
Commit 0679ca0

@github-actions
Copy link

github-actions bot commented Jul 28, 2023

Deploy preview for hack-ics-uci-edu-sanity ready!

Name Sanity Studio
Preview Visit Preview
Commit 0679ca0

Copy link
Contributor

@samderanova samderanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Another thing that we could take care of on this PR is changing the LinkedIn icons on the top right of each image to be non-transparent like the ones on the current Hack site. Otherwise, they could be hard to see:

image

apps/site/src/app/about/getMembers.ts Outdated Show resolved Hide resolved
@waalbert
Copy link
Author

Thanks for working on this!

Another thing that we could take care of on this PR is changing the LinkedIn icons on the top right of each image to be non-transparent like the ones on the current Hack site. Otherwise, they could be hard to see:

image

Switched the LinkedIn icons to the ones used on the current Hack site.
image

Comment on lines 7 to 10
`*[_type == 'boardYear'][0]{corporate[]{person->{name, profilePic{asset->{url}}, socials[0]{link}}, position},
logistics[]{person->{name, profilePic{asset->{url}}, socials[0]{link}}, position},
marketing[]{person->{name, profilePic{asset->{url}}, socials[0]{link}}, position},
tech[]{person->{name, profilePic{asset->{url}}, socials[0]{link}}, position}}`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This groq query can probably be condensed a bit.

Something like the following should work since we don't need the asset url anymore.

*[_type == 'boardYear'] | order(year desc) [0]{
  corporate[]{
    person->{
      name,
      profilePic,
      socials[0] {link}
    },
    position
  },
  logistics[]{
    person->{
      name,
      profilePic,
      socials[0] {link}
    },
    position
  },
  marketing[]{
    person->{
      name,
      profilePic,
      socials[0] {link}
    },
    position
  },
  tech[]{
    person->{
      name,
      profilePic,
      socials[0] {link}
    },
    position
  }, 
}

Copy link
Contributor

@alexanderl19 alexanderl19 Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't catch this the first time around, but we probably shouldn't assume the LinkedIn link is the first one in the array. I'll come up with an updated groq query later and reply here.

apps/site/src/app/about/getMembers.ts Outdated Show resolved Hide resolved
apps/site/src/app/about/TeamSection.tsx Outdated Show resolved Hide resolved
apps/site/src/app/about/TeamCard.tsx Outdated Show resolved Hide resolved
apps/site/src/app/about/TeamCard.tsx Outdated Show resolved Hide resolved
apps/site/src/app/about/getMembers.ts Outdated Show resolved Hide resolved
apps/site/src/app/about/getMembers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@alexanderl19 alexanderl19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just some minor fixes!

apps/site/src/app/about/getMembers.ts Show resolved Hide resolved
apps/site/src/app/about/page.tsx Outdated Show resolved Hide resolved
@alexanderl19 alexanderl19 self-requested a review August 13, 2023 00:16
Copy link
Contributor

@alexanderl19 alexanderl19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last change - everything else looks good!

_type,
name,
profilePic,
socials[0] {link}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replacing socials[0] with socials[platform == 'linkedin'][0] should ensure that we fetch the member's LinkedIn social.
This is essential filtering the socials array for a value where platform is 'linkedin', then selecting the first result that's found. It's not dissimilar from what we're doing on L35, where we're filtering then selecting the first result.

Comment on lines +19 to +20
person: Person,
position: z.string(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: why is position separate from Person? Wouldn't it make more sense for position to be an attribute of the Person? Or is this to handle a person being part of multiple departments and be able to reuse a Person? Probably uncommon, but in such a case, someone might want different profile photos per department?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is focused on pulling content from Sanity, so the Zod schema here is just validating what's returned from Sanity.

As far as I can remember, we decided to separate the Person object from organizers to make it more reusable. For example, people may switch to different departments between years, or we may decided to use the Person object as an author field in the future. The profile photo was something we discussed at the time, but ultimately decided that it wasn't worth the extra complexity. Might be something we have to revisit in the future?

cc @samderanova

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding on to what Alex already mentioned, there might be former organizers that we might want to display on other sections of the site in the future (ex. alumni showcases). In situations like those, it would be better to just have one schema for all of our people and then use them in other schemas where needed instead of having two schemas to do the same.

apps/site/src/app/about/getMembers.ts Show resolved Hide resolved
Comment on lines -5 to +6
import linkedinLogo from "@/lib/common/assets/icons/linkedin.svg";
import linkedinLogo from "@/lib/common/assets/linkedin-logo.svg";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chore: if we're replacing the logo icon, could we remove the older version.

apps/site/src/app/about/TeamCard.tsx Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: this asset seems quite large, is this official or made with a vectorizer? The color is also slightly off from the LinkedIn Blue. Would prefer to avoid diverging from brand guidelines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Organizer Page
4 participants