-
Notifications
You must be signed in to change notification settings - Fork 55
Portfolio Project - Gabriella #44
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
sandrahagevall
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 great job with your portfolio — it looks very good, and I really like the animations you've added! You’ve also done very advanced work with all of the components, and it’s clear that you’ve put a lot of thought into the overall structure.
One thing that could make the project even easier to navigate for someone who hasn’t seen the code before is the folder structure. Since there are many components, styles and assets, it can be a little challenging at first to understand what belongs together. Maybe grouping related files in shared folders could make it even clearer.
Overall, you’ve done an excellent job — the code is clean, structured, and very well written 💯
| const [visible, setVisible] = useState(false); | ||
|
|
||
| // to change threshold value depending on screen size | ||
| const isBigScreen = window.matchMedia("(min-width: 768px)").matches; |
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.
You could have used your breakpoints from the theme here! :)
| @@ -0,0 +1,11 @@ | |||
| import React from 'react'; | |||
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.
This file feels a bit misplaced, as it's a component and not a style element per se. The style folder should probably be put outside of the components folder.
| // firsly check if size/weight values have been put manually, otherwise use pre-defined values for media size: | ||
|
|
||
| // mobile (as default) | ||
| font-size: ${({ as, size }) => size ? sizeMapping[size] : typographyConfig[as]?.mobile.size}; |
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 use of styled props!
| <Typography as="h2">Skills</Typography> | ||
| <StyledSkillsContainer> | ||
| {skillsData.skills.map((skillObj, index) => ( | ||
| <SkillsBox key = {index} skillObj = {skillObj} /> |
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.
minor thing, but it looks weird to put spaces between the prop name and the prop value like this.
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.
Good use of props and the theme, good structure for most of the parts and easy to read. Great job!
Netlify: https://gabriellaberkowicz.netlify.app/
Figma Design: https://www.figma.com/design/eVfASpgdDyyWXVDaTilAER/Portfolio-designs--Copy----Gabriella?node-id=0-1&p=f&t=c6f0xYut6i6DJPJ3-0