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

Feature/previous projects #71

Merged
merged 47 commits into from
Aug 5, 2024
Merged

Feature/previous projects #71

merged 47 commits into from
Aug 5, 2024

Conversation

Jordan231111
Copy link
Contributor

Please briefly describe the changes made in your pull request below

Implemented the entire previous projects from 2023 page

Known Issues

  • Mobile scaling
  • next js routing

If your changes made visual updates to the website, please add screenshot(s) below

image

@Jordan231111 Jordan231111 linked an issue Jun 6, 2024 that may be closed by this pull request
Copy link
Member

@CooperW824 CooperW824 left a comment

Choose a reason for hiding this comment

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

So this looks pretty good so far. Thank you! For components like the carousel I usually prefer to find a library that does it for me so that I don't have to worry about all the complicated logic of the component and making sure it sizes dynamically. Also do you think you could add the prize categories to the site? So that visitors see which projects won what.

You might want to check out this cool component I found on NPM: https://www.npmjs.com/package/react-multi-carousel

<div className="flex flex-col items-center h-full mx-8">
{" "}
{/* Added horizontal margins here */}
<img
Copy link
Member

Choose a reason for hiding this comment

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

I would replace this with Next's Image component, just for consistency and image optimization.

Copy link
Member

Choose a reason for hiding this comment

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

You could remove this file if you switch to pre-built component

@Jordan231111
Copy link
Contributor Author

So this looks pretty good so far. Thank you! For components like the carousel I usually prefer to find a library that does it for me so that I don't have to worry about all the complicated logic of the component and making sure it sizes dynamically. Also do you think you could add the prize categories to the site? So that visitors see which projects won what.

You might want to check out this cool component I found on NPM: https://www.npmjs.com/package/react-multi-carousel

Is there a way to do it without the library that seems more complicated and also not sure how I would use that and still match the figma design.
I am also confused how I would add prize categories where would it go?

@CooperW824
Copy link
Member

The way that image carousels like this one are implemented are usually done by having a parent div with a fixed width and the css property overflow-x: hidden. Inside that div you have a list of all the carousel elements, each element has the same width as the parent div. Then when the user clicks a button, you just set the translation (x-movement) of all the carousel items to be offset left or right by the width * visible_index (i.e. which item is visible). Your approach also works well, if you wanted to get fancy without changing too much of the logic, you could do a fade-out, fade-in type of thing without adding too much logic using requestAnimationFrame().

Another thing that is important the Figma designs are not the end all be all, they are a suggestion for the layout, but ultimately the developer has the final say on the layout and design. If you want you could move the buttons, change their sizes, change colors, etc. I strongly encourage you to change the designs as the design for the buttons on Figma was not supposed to be like that on the site.

Lastly, about the prize categories, I would recommend placing them under the project title in maybe smaller font? Ultimately its up to you as the dev.

@CooperW824 CooperW824 merged commit 9b5d43f into main Aug 5, 2024
1 check 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.

Past Years Projects
3 participants