Portfolio #30
Portfolio #30
Conversation
There was a problem hiding this comment.
Overall, the website looks great! I really like the styling and especially the color scheme! Great work!
I understand that you have been sick and have not been able to finish everything, but I think overall the code is clear and understandable. The structure of the files and use of components are easy to overview. However, you could maybe break out some code into more components, especially in the projects section where there is a lot of repeating code for each project. For example, the project card could be a separate component with props for the project info :)
I checked your light house score and it's 85%. The score could be improved by adding alt attributes to images and to make sure that heading elements is in the descending order (I think it's mainly in the projects section where you should look over the headings).
I also looked at the Figma design and I think you have done a really good job on implementing the design of the home and tech sections in the finished website :)
Keep up the good work Selin!
src/components/Home.jsx
Outdated
| <img | ||
| src={selin} | ||
| style={{ | ||
| height: "500px", | ||
| borderRadius: "500px", | ||
| }} |
There was a problem hiding this comment.
Add an alt attribute to the image to improve accessibility :)
src/components/Navlinks.jsx
Outdated
| `; | ||
|
|
||
| const Atags = styled.div` | ||
| width: 900px; |
There was a problem hiding this comment.
I noticed that the menu bar is wider than the screen width (most visible on mobile). This is probably because this div has a fixed width of 900px. A suggestion to fix this is to remove the width and instead add a gap to keep the links separated
src/components/Projects.jsx
Outdated
| `; | ||
|
|
||
| const Wrapper = styled.div` | ||
| width: 900px; |
There was a problem hiding this comment.
This fixed width will make the div wider than the width of smaller screens. I understand that you are not finished with media queries to make the site responsive, just a reminder here to code mobile first :)
| gap: 80px; | ||
| `; | ||
|
|
||
| const Title = styled.h1` |
There was a problem hiding this comment.
I think it's best practice to only have one h1 on the website, so I think this title should be a h2. Changing this would probably improve the lighthouse score too
src/components/Projects.jsx
Outdated
| gap: 50px; | ||
| `; | ||
|
|
||
| const H2 = styled.h2` |
There was a problem hiding this comment.
Changing this heading to h3 will probably improve the lighthouse score
src/components/Projects.jsx
Outdated
| color: black; | ||
| `; | ||
|
|
||
| const InfoBox = styled.h3` |
There was a problem hiding this comment.
This probably shouldn't be a heading but a div instead?
src/components/Projects.jsx
Outdated
| const Image1 = styled.img.attrs({ | ||
| src: image1, | ||
| })` | ||
| ${imageStyles} | ||
| `; | ||
|
|
||
| const Image2 = styled.img.attrs({ | ||
| src: image2, | ||
| })` | ||
| ${imageStyles} | ||
| `; | ||
|
|
||
| const Image3 = styled.img.attrs({ | ||
| src: image3, | ||
| })` | ||
| ${imageStyles} | ||
| `; | ||
|
|
||
| const Image4 = styled.img.attrs({ | ||
| src: image4, | ||
| })` | ||
| ${imageStyles} | ||
| `; | ||
| const Image5 = styled.img.attrs({ | ||
| src: image5, | ||
| })` | ||
| ${imageStyles} | ||
| `; |
There was a problem hiding this comment.
These images are missing alt atributes. Add for improved accessibility :)
There was a problem hiding this comment.
You have a little bit more work to do:
- follow the design
- responsiveness
- a11y
Remember going forward to try to keep your code DRY and utilise everything within JS (such as map()) and React (reusable components and props). But your code looks good, so that requirement is ✅
Keep up the good work and let us know if you need help with anything ⭐
A link to your Figma design
https://www.figma.com/design/vn1fOrwqD8rUk1faGppsVg/Selin-Portfolio--Copy-?node-id=2026-528&m=dev
and a Netlify link.
https://selincarbone.netlify.app/
I had a flu so Im a little behind in the project, but Its a living art and I will contine to build on the portfolio.
What needs to be done is connect the links to the icons.
Write the correct description on the projects and also finish the mediaqueries.
In this project I had to try out React.js for the first time and build it together with some HTML, CSS and Javascript.
Thank you for watching my profile.
Best regards
Selin Carbone