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

feat: dynamic routing for each event on MyEvent page #25

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

Conversation

aidenm1
Copy link
Contributor

@aidenm1 aidenm1 commented Nov 7, 2024

🍞 What's new in this PR

🥐 Description

Added dynamic routing for each event in my event page.

🥖 Screenshots

image

🥪 How to review

Review the querying functions within api/supabase/queries. Specifically, events, facility, and volunteers. (I believe these functions are good)

Next, review app/events/[event_id]/page.tsx and app/events/[event_id]/styles.ts. I'm not sure if I did the styling the conventional way, but it seems to work.

🥧 Next steps

image In Jinkang's design, the top of the content area is curved, I'm not entirely sure how to do this.

Will phone numbers be formatted in the database, or should I create a function to format the phone numbers so that they are (xxx)xxx-xxxx?

Should second contact be either Volunteer Host or Facility Host depending on needs_host bool, or should it just be Volunteer Host?

Can we guarantee that there will always be a volunteer host who signed up and a performer who signed up? If not guaranteed, should there be a separate page rendered saying that event details haven't been finalized yet?

🥞 Relevant links

🥨 Online sources

🥯 Related PRs

CC: @celinechoiii

@celinechoiii
Copy link
Collaborator

celinechoiii commented Nov 8, 2024

to answer your questions:

  • it would be nice if you could create a function to format the phone numbers! you can put this in the utils folder
  • if the needs_host boolean is false, the second contact will be the facility host. if true, the volunteer host contact will be listed
  • for now, we can assume that there will always be a host or performer who signed up! but that is a good point. i will relay this to jinkang and ask his thoughts

overall, amazing job!! once the changes are made, i should be able to merge!

object-fit: cover;
`;

export const GradientOverlay = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think you can get rid of the GradientOverlay styling

margin: 1rem;
`;

export const ImageWrapper = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

ImageWrapper can be deleted as well!

display: flex;
`;

export const EventImage = styled(NextImage)`
Copy link
Collaborator

Choose a reason for hiding this comment

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

export const EventImage = styled(NextImage)`
  width: 100%;
  height: 23vh;
  object-fit: cover;
  position: relative;
`;

);
`;

export const Page = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

i set the default background to be gray1

export const Page = styled.div`
  flex-direction: column;
  min-width: 100%;
  min-height: 100svh;
  overflow: hidden;
`;

margin-bottom: 3.75rem;
`;

export const Container = styled.div`
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • border-radius field helps us round the corners!
  • deleting margin-bottom field in Page since bottom padding is directly being added to this container
  • negative margins help us overlap elements and z-index sets the order of overlapped elements. the default z-index value is 0 and higher z-index values bring elements to the front, while lower values push them to the back
  • i'm making the background white for now so that the container doesn't blend in with the default background color
export const Container = styled.div`
  z-index: 10;
  margin: -1.5rem 0;
  position: relative;
  padding: 1.5rem 2rem 4rem 2rem;
  display: flex;
  flex-direction: column;
  border-radius: 20px 20px 0 0;
  background: white;
`;


import NextImage from 'next/image';
import styled from 'styled-components';
import COLORS from '@/styles/colors';
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete this line as no global colors are being used

Comment on lines +85 to +88
<styles.ImageWrapper>
<styles.EventImage src={BPLogo} alt="EventImage" />
<styles.GradientOverlay />
</styles.ImageWrapper>
Copy link
Collaborator

Choose a reason for hiding this comment

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

delete these are replace with this:
<styles.EventImage src={BPLogo} alt="EventImage" />

import { UUID } from 'crypto';
import supabase from '../createClient';

export async function fetchPerformer(event_id: UUID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

make this function name a bit more descriptive! something like fetchPerformerByEventID

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.

2 participants