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

Event stats #36

Open
wants to merge 41 commits into
base: dev
Choose a base branch
from
Open

Event stats #36

wants to merge 41 commits into from

Conversation

isa-leroux448
Copy link

Changes

  • added bar charts, pie charts and stats tables to event stats page according to Figma
  • created a percentage bar chart
  • updated mock DB data type to match data currently stored in our registrations database
  • edited stats table to work with same input data type as bar chart and pie chart

Out Of Scope

  • backend connection

Notes

  • I had some issues with the react google charts responsiveness where they wouldn't scale when I resized the screen. I added a function that reloads the charts on screen resize for both the pie chart and bar chart
  • I don't think we store school value in our registration data so I omitted that chart
  • Also had some styling issues related to conflicts between tailwind and inline styling using dynamic widths for the chart boxes so I ended up using a CSS module for simplicity

Image

Desktop1 Desktop2
Mobile.mov

Copy link

vercel bot commented Aug 15, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
biztech-v2 ❌ Failed (Inspect) Aug 26, 2024 5:27am

@isa-leroux448
Copy link
Author

I ran into a slight type issue. I created a new type called "AttendeeDetailedInformation" based on the registration data in the DB. I edited the return value of the fetchRegistrationData function to use this type. However, I realize that we are using quite a few different types (e.g. type Attendee) to represent attendee data and I'm not sure if they should all use the same type? It's causing a type error in the check but I'm hesitant to change any types and mess up another part of the code.
Any recommendations? Should we consolidate the different types into one?


data.forEach((item) => {
const { label, value } = item;
percentages[label] = (value / total) * 100;
Copy link
Contributor

Choose a reason for hiding this comment

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

is total always gonna be non zero?

};

export type AttendeeDetailedInformation = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Types should be consistent with db. So i think this here should be "Registration"

@AllanT102
Copy link
Contributor

AllanT102 commented Aug 17, 2024

I ran into a slight type issue. I created a new type called "AttendeeDetailedInformation" based on the registration data in the DB. I edited the return value of the fetchRegistrationData function to use this type. However, I realize that we are using quite a few different types (e.g. type Attendee) to represent attendee data and I'm not sure if they should all use the same type? It's causing a type error in the check but I'm hesitant to change any types and mess up another part of the code. Any recommendations? Should we consolidate the different types into one?

It's best to be consistent yes. I did a quick look and it should be fine using AttendeeDetailedInformation if that represents the registration information. currently fetchRegistrationData is used for the registration table on admin side and the stats page, so it won't be a large change.

Please change this field to be "Registration" so that it matches the db table name.

Also, the fetchBackend function is on dev now and it works. Please implement the call to the backend and fetch real data.

@isa-leroux448

@voctory
Copy link
Member

voctory commented Aug 17, 2024

I ran into a slight type issue. I created a new type called "AttendeeDetailedInformation" based on the registration data in the DB. I edited the return value of the fetchRegistrationData function to use this type. However, I realize that we are using quite a few different types (e.g. type Attendee) to represent attendee data and I'm not sure if they should all use the same type? It's causing a type error in the check but I'm hesitant to change any types and mess up another part of the code. Any recommendations? Should we consolidate the different types into one?

It's best to be consistent yes. I did a quick look and it should be fine using AttendeeDetailedInformation if that represents the registration information. currently fetchRegistrationData is used for the registration table on admin side and the stats page, so it won't be a large change.

Please change this field to be "Registration" so that it matches the db table name.

Also, the fetchBackend function is on dev now and it works. Please implement the call to the backend and fetch real data.

@isa-leroux448

Hey @AllanT102, do we need to get the backend functionality working in this PR? It might be better to merge our efforts to fetch event and registrations data on both the Stats page and the Admin Events Registrations page, otherwise we're duplicating the amount of work to get both pages working.

@AllanT102
Copy link
Contributor

I ran into a slight type issue. I created a new type called "AttendeeDetailedInformation" based on the registration data in the DB. I edited the return value of the fetchRegistrationData function to use this type. However, I realize that we are using quite a few different types (e.g. type Attendee) to represent attendee data and I'm not sure if they should all use the same type? It's causing a type error in the check but I'm hesitant to change any types and mess up another part of the code. Any recommendations? Should we consolidate the different types into one?

It's best to be consistent yes. I did a quick look and it should be fine using AttendeeDetailedInformation if that represents the registration information. currently fetchRegistrationData is used for the registration table on admin side and the stats page, so it won't be a large change.
Please change this field to be "Registration" so that it matches the db table name.
Also, the fetchBackend function is on dev now and it works. Please implement the call to the backend and fetch real data.
@isa-leroux448

Hey @AllanT102, do we need to get the backend functionality working in this PR? It might be better to merge our efforts to fetch event and registrations data on both the Stats page and the Admin Events Registrations page, otherwise we're duplicating the amount of work to get both pages working.

True... ok let's do that @isa-leroux448 can you handle this then? Once you merge this in, I've assigned you the ticket to connect backend for registration table and stats

@voctory
Copy link
Member

voctory commented Aug 25, 2024

PR LGTM, will approve once the Vercel build passes

@isa-leroux448
Copy link
Author

isa-leroux448 commented Aug 26, 2024

I fixed the userInfo component used in the attendee information popup to use the new Registration type and be able to handle the nested user profile information. Right now I'm running into issues when building with next but I don't think they're related to my branch. I think some broken stuff got merged into dev because the package.lock syntax is off and I get a whole bunch of errors like these when I run next build:
Error occurred prerendering page "/admin/home". Read more: https://nextjs.org/docs/messages/prerender-error
and
Error: NextRouter was not mounted. https://nextjs.org/docs/messages/next-router-not-mounted

@isa-leroux448
Copy link
Author

Is this a problem with the other PRs? Wondering if it's something I should fix in my branch or another bug fix ticket altogether

@AllanT102
Copy link
Contributor

I fixed the userInfo component used in the attendee information popup to use the new Registration type and be able to handle the nested user profile information. Right now I'm running into issues when building with next but I don't think they're related to my branch. I think some broken stuff got merged into dev because the package.lock syntax is off and I get a whole bunch of errors like these when I run next build:

please fix alongside this PR so it passes build, thanks

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.

4 participants