-
Notifications
You must be signed in to change notification settings - Fork 55
Portfolio - Asako #39
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -1,8 +1,44 @@ | |||
| import { useEffect } from "react" | |||
| import AOS from "aos" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed that this, aos, is a really old library (7 years since last publish). Avoid old packages, this is due to security concerns.
| @@ -0,0 +1,15 @@ | |||
| import { createGlobalStyle } from "styled-components" | |||
|
|
|||
| export const GlobalStyle = createGlobalStyle` | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one thing to note for future projects is to use max-width: 100%; on images in the global styling, then the images wouldnt overflow their parent, like it does in your mobile version :). Like this:
img {
max-width: 100%;
}| @@ -0,0 +1,6 @@ | |||
| import styled from "styled-components" | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The theme file should be used for general reusable sizes, colors, media queries and such :)
| @media (max-width: 744px) { | ||
| .slide { | ||
| width: 300px !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using !important should be a last resort. In this case, it looks like, you want to override the css from swiper?
Duplicate the classname instead to increases the specificity in order to overwrite it. Like this:
.slide.slide {
width: 300px;
}| ) | ||
| } | ||
|
|
||
| const imgMap = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice that you use a map of all the images in order to get it via the name in the project object.
Npahlfer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good and easy to follow along. I like that you make use of maps to get the images. I would have wanted to see some use of a theme file for colors, media queries and such. Otherwise good job!
julialindstrand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really professional and nice looking portfolio! You got a lot of cool animations and it was never "too much". I really like the color and the readability. It was also a nice and clean code that was easy to understand! Great job! A first class portfolio :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hero looks great and really nice animation! Just a little to much space between the rows of text for my liking but that's individual :)
| ] | ||
| }, | ||
| { | ||
| "title": "Toolebox", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's an E too much here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I'm nitpicking but according to the design the buttons should be different from eachother but this looks really clean!
|
|
||
| // Styled component below | ||
|
|
||
| const JourneySection = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe put the articles more centered since they are fewer then the projects? So it looks more intentional :)
|
|
||
| // Styled component below | ||
|
|
||
| const ContactContainer = styled.div` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again a personal preference but maybe it's to wide a gap between the text and the picture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the design there were two different fonts but I don't think its noticeable on your page
Netlify link - https://asako-portfolio.netlify.app/
Figma link - https://www.figma.com/design/8rhyek3LxsIZM2rjSBVfee/Portfolio-design?node-id=0-1&t=3RJwp8MXaEhHBVpV-1