Portfolio Danu - Code review #21
Conversation
There was a problem hiding this comment.
- Some of the things i noticed on the deployed site is that you have "View the codeCode" on the button.
- Media Querys
- The first and second column under Skills are not aligned.
Thank you for letting me review your code Danushka! Great job!! I think the structure of the whole page/project is superclear and well thought out. I dont find much to improve other then look at intendings and spaces between section/lines and maybe delete unnesseccary comments.
I uploaded som pictures, and i can se that you are working with media querys but it dosen't seem that your done with this? I posted some from the mobile view.
Consider to install a extension for the format so it will save and format automatically :) It will help you keep consistency and a clean code through the whole project.
I also wonder if the rolling footer at the end of the page could go a little bit slower? I get a little bit dizzy when its rolling to fast haha! Couldn't barely read it unforunately.
Other than that, amazing!! You have so much cool projects that fits supergood in your portfolio. I would defenitely hire you if i had a company :D ❤️
src/components/Projects/Projects.jsx
Outdated
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content:center;width: 100%; |
There was a problem hiding this comment.
Think about the format. I should've put width on a second line, this is probably something you missed but thats why im letting you know :)
src/components/Skills/Skills.jsx
Outdated
| flex-wrap: wrap; | ||
|
|
||
| padding: 2rem; | ||
|
|
There was a problem hiding this comment.
Also put this together, i dont know whats best practise, but since it is under the same styled.div i think it looks cleaner if they're put together
| text-align: center; | ||
| margin-bottom: 2rem; | ||
| width: 100%;color: #e0e0eb; | ||
| `; |
| display: flex; | ||
| flex-wrap: wrap; | ||
| gap: 1rem;flex-direction:column; | ||
| `; |
src/components/Tech/Tech.jsx
Outdated
| return ( | ||
| <Element name="tech"> | ||
|
|
||
|
|
There was a problem hiding this comment.
maybe to much gap between here? Erase one line maybe? :)
There was a problem hiding this comment.
Love this picture! It's good quality and you look so happy :D
src/components/Article/Article.jsx
Outdated
| const Title = styled.h2` | ||
| font-size: 2rem; | ||
| font-weight: bold; | ||
| text-align: left; /* Make sure text aligns to left */ |
There was a problem hiding this comment.
Now im picky sorry, i think this "text-align" speaks for its self that the text is align to left. So maybe delete this comment for cleaner code? :)
| } | ||
| `; | ||
|
|
||
| const Image = styled.img` |
| } | ||
| `; | ||
|
|
||
| const ContactTitle = styled.h2` |
There was a problem hiding this comment.
Consider to install prettier as a format in Vs code to have this sort of things fixed automatically? :) font-size is one or two more tabs according to color.
src/components/NewCard/Card.jsx
Outdated
There was a problem hiding this comment.
Is there a meaning why you have named it newcard? Could it be more explained wich section its belongs to? :)
There was a problem hiding this comment.
A11y
Background and foreground colors do not have a sufficient contrast ratio.
Regarding the requirement: Your portfolio should follow the Figma design
- Something is wrong with your fonts. Make sure you import them properly.
- And make sure to thoroughly follow all parts of the design such as fonts and spacings. Top/bottom padding of sections should be consistent according to the design.
The code is structured and you’re utilising components and props in a good way 👍
Changes requested
- Follow the design more thoroughly
- A11y (contrasts)
|
Still some changes needed to follow the design! |
|
Ping! 🔔 |
(https://danushkasoamsiri.netlify.app/)
Custom Domain : http://danushkasomasiri.com/
https://www.figma.com/design/31cfXywpNds0IanPHgwJqR/Portfolio-Danushka?node-id=0-1&p=f&t=ZwRiCgvv3edjaRtz-0