Portfolio #398
Portfolio #398
Conversation
There was a problem hiding this comment.
Fantastic job with the portfolio, Gitte! 🌟 The code was really easy to follow and read. I left a couple of comments on potential improvements, but overall it looks really great.
I can see some things that differ from the design:
- The thing that I noticed first is that you didn't add a max-width to the grid. In the design it looks like all sections except projects and the articles follow the grid. Setting
max-width: 1084px;andmargin: 0 auto;to .grid-section should do the trick. You might have to adjust some other stuff though because of the change. - HeaderTwo has the wrong size on desktop.
- The body font-size for the tech stack is too small and it doesn't seem to be a max-width here either. Looking at the design it seems it should span 8 columns of 12 on desktop.
- The footer has the wrong layout on desktop. I think it might be because you placed a section inside the .grid-section instead of having the two divs with the footer content as direct children of .grid-section.
- Not related to the design, but I think it would be nice to at least add
cursor: pointer;to the buttons so that they look clickable when hovering. Some sort of hover state would be nice too.
It was inspiring to look through you code and see how you approached different sections. I might just steal some of it for future projects 👏
There was a problem hiding this comment.
I find the name MediaButton and MediaButtonContainer a bit unclear. To make it more readable I would suggest renaming it to SocialMediaButton, even though that's longer.
src/App.jsx
Outdated
There was a problem hiding this comment.
Great structure, so easy on the eyes and mind 😍
There was a problem hiding this comment.
I like your solution for how to enter the data here! I went with a JSON-file, but it feels a bit excessive when there is not that much data, so this was cleaner I think.
There was a problem hiding this comment.
I was expecting to see the map method in the ProjectsSection.jsx and not here in the ProjectCard component. If you ever wanted to make this a reusable component I would suggest moving that logic outside the ProjectCard.jsx and pass the data in via props instead.
There was a problem hiding this comment.
Also noticed a console.log still present in the code. Maybe remove that if you don't need it anymore.
src/App.css
Outdated
There was a problem hiding this comment.
It says this file is empty, so maybe you should remove it?
src/ui/Grid/Grid.css
Outdated
|
|
||
| @media(min-width: 1024px) { | ||
| .grid-section { | ||
| grid-template-columns: (12, 1fr); |
There was a problem hiding this comment.
The repeat function is missing in grid-template-columns: (12, 1fr);, so it never gets applied on desktop.
There was a problem hiding this comment.
You seem to be mixing CSS Grid and FlexBox attributes here in a way that I am not sure actually works. In the browser it looks like .bio-header { grid-column: span 4; } is not getting applied because the parent is Flex for instance.
- set max-width
- worked on footer section
- added typo for body text
https://portfolio-gitte.netlify.app/