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

CW2 28 create modal on sponsor click #16

Merged
merged 13 commits into from
Jun 27, 2024

Conversation

QuadAces
Copy link
Collaborator

Why the changes are required?

Because we want a modal to show up when we click on a sponsor!

Changes

  • added a setstate to keep track of a modal
  • added a setstate to keep track of modal information (yes I could use useReducer or I could just not)
  • changed sponsor TS interface to have a description
  • added modal component

Screenshots

Screenshot 2024-06-20 at 8 47 43 pm
Screenshot 2024-06-20 at 8 47 54 pm
Screenshot 2024-06-20 at 8 48 07 pm

Comments

May I say that the datastructure to keep track of our sponsors is awful. Since we have objects for each sponsor level (const diamond = {} and also have const gold = {}) I think this is really hard to work with as I had to manipulate each of the components mapped to show a modal separately. I also suggest adding a name to the sponsors datastructure field also.

Lastly, there are some errors with the source for some of the company logos and svgs. I understand Ming is working on this, just note that some svgs/images dont show on click for the modal.

@QuadAces QuadAces requested a review from derekxu04 June 20, 2024 10:48
Copy link
Collaborator

@derekxu04 derekxu04 left a comment

Choose a reason for hiding this comment

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

Nice work, looks really good!

Just some changes:

  • I've noted some changes above on specific lines
  • Would be nice to either indicate that the logo is clickable in the modal, or to just have a "Learn more" button / text or something, otherwise it is not super obvious

Comments:

  • Do you know why some of the svgs don't show up?
  • With regards to the format of data.ts, I don't see any way of avoiding having to split the sponsors into the three sections. If we want to avoid repeating the code to do with the modal, we can make it even more modular and make a SponsorSection component or something for each level, which would work I think, or maybe just write a function that does that for you without needing to make it its own entire new component
  • Agree with adding the name property to each company, either you can do that or I'll can do that at some point

frontend/src/components/Sponsors/sponsorModal.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorModal.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorModal.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorModal.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorlinks.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorModal.tsx Outdated Show resolved Hide resolved
frontend/src/components/Sponsors/sponsorlinks.tsx Outdated Show resolved Hide resolved
@QuadAces
Copy link
Collaborator Author

Okay great, done pretty much everything. for data ts, I just propse that we have an object with 3 key values just to at least connect the data somehow e.g. {diamond sponsors: [], gold sponsors: []}
I obviously dont want to make things more complex than they should be, just think they should be grouped together. I've added everything else :P

@derekxu04
Copy link
Collaborator

Okay great, done pretty much everything. for data ts, I just propse that we have an object with 3 key values just to at least connect the data somehow e.g. {diamond sponsors: [], gold sponsors: []}

Yep that makes sense I'll do this in another ticket

@derekxu04 derekxu04 merged commit ca6c1c9 into master Jun 27, 2024
2 checks passed
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