Sofie Portfolio #11
Sofie Portfolio #11
Conversation
simfrisk
left a comment
simfrisk
left a comment
There was a problem hiding this comment.
Really nice site Sofie, really! I love that it works on the smallest of phones up to larger screens. Really easy to read the code with great naming and structure. It feels like you understood the props, components, styled-components and all the react stuff really well. And most important, everything works!
I think you have a great design although I know you said that you would like to change it. I especially liked the navbar so that you can get to the part you want quickly and easaley. Nice that you got the scroll animation to work.
Some things that potentially could be improved was
- Some layout sizing not perfect on desktop. Like the height being just off.
- The project cards on desktop could be more evenly divided in columns and rows.
- Maybe add a image for the blog, if you even will use it.
Other than that I think you did a super nice job and I hope that you are really proved of what you achieved, I know I would have been!
/Simon
There was a problem hiding this comment.
Looks great! A understandable and clear structure!
There was a problem hiding this comment.
I like it but might be a good idé to have a unorded list as well and a nav tag and so on for semantics. I personally feel like you did it cleaner but I think it might be better to have it
There was a problem hiding this comment.
It all looks really great! Just one thought, should not the cards be an article and not a section since the cards are already in a section?
| <ProjectHeading id="projects">Featured Projects</ProjectHeading> | ||
|
|
||
|
|
||
| <ProjectsContainer> |
There was a problem hiding this comment.
Since this is a section I think the cards could be an article or something.
| export const Projects = () => { | ||
| return ( | ||
| <> | ||
| <ScrollAnimation> |
There was a problem hiding this comment.
Is this for the header button scroll? I really like it! I so great that you can get quickly to the different section
| import styled from 'styled-components'; | ||
| import { Icons } from './components/Icons'; | ||
|
|
||
| const HeaderContainer = styled.header` |
There was a problem hiding this comment.
I like it but in my preference I wish that the content could be a bit more centered.
src/assets/sections/Header.jsx
Outdated
| padding-top: 100px; | ||
|
|
||
| @media (min-width: 768px) { | ||
| height: 100vh; |
There was a problem hiding this comment.
Maybe it would work is this was 100dvh so that it takes the nav into the calculations.
There was a problem hiding this comment.
Great layout and well executed with all the positioning of all the different elements.
src/assets/sections/Blog.jsx
Outdated
There was a problem hiding this comment.
I wont use this section :)
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
A11y
100% in Lighthouse ⭐
Regarding the requirement: Your portfolio should follow the Figma design
- You have only four projects in your design, so there’s no decision around what to do with a potential fifth but I would strongly suggest that you left-align it. Also, when the projects span over two rows, I think they need a little more space in between them.
- Another thing to look into is to align all your sections. In your design, all sections have different spacing (left/right) and I’m not sure it’s deliberate?
- I think it might become harder to maintain when you have a lot of different paddings and margins, e.g. on the Project section, the Card container has a margin and a padding around the whole thing + the Project container also has its own padding. I think you should be able to get rid of some of these. Also, maybe read up on the difference between padding and margin? 😇
- I think the top and bottom sections are on point ⭐
Codewise
What’s the purpose of putting the sections in your assets folder? Assets are e.g. images and such, so a folder called sections right in the src folder should be fine.
I think it would be nice if you mapped through the projects array in the Projects.jsx instead. Since Card.jsx is named in singular, I’d expect to find a single card there, but instead you’re mapping through all the cards there. Update this or the naming so that it’s clearer.
Overall you’ve done a good job structuring your project though, it’s easy to overview.
Changes requested
- Have a look at the comments I wrote about the design
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Projects still don’t look like the design, see my comment from before: when the projects span over two rows, I think they need a little more space in between them.
|
Ok I have added more space and changed justify-content to flex start to left align any added projects under Projects section. |
4 participants
https://ssofiejohansson.netlify.app/
https://www.figma.com/design/0VCk5KGQ5KRujnjPcXjPzH/Figma-designs-for-students--Sofie-?node-id=3092-1048&m=dev