Conversation
… created ui for button and card. Updated App.jsx with imports and exports
gittebe
left a comment
gittebe
left a comment
There was a problem hiding this comment.
Great work, Anna! You've created a responsive portfolio built with React components. I really liked the heading levels you used — great idea! Maybe you could add one for the paragraph element as well?
I think your code across the different files is easy to follow. For the overall file structure, it might help break up the "ui" folder a bit by separating individual components (such as the button or the header) from those components that already combine ui elements. This could make it easier to navigate as the project grows. :)
Great work, Anna! Keep it up!
| ) | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
I think I would delete the comment showing the structure. It is not the final structure you are using and it could be a bit confusing :)
| <Contact /></> | ||
| ) | ||
| } | ||
|
|
There was a problem hiding this comment.
That looks great! You have components for every section. Nice! I would give the <> and </> an extra line :)
| "github": "link" | ||
| } | ||
| ] | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nicely structured and I like that you already added the "tags". (Need to do that as well :))
There exists another empty projects.json file. Not sure if you plan to use it. If not maybe you could delete it?
| font-weight: 500; | ||
| margin-bottom: 0.8rem; | ||
| } | ||
|
|
There was a problem hiding this comment.
Only one thing coming to my mind here because I did the same ;) You gave the heading3 a margin-bottom. Sometimes you like to use the heading 3 without this margin. A suggestion would be to not defining it within this ui but instead add margin to those components where the heading is used.
| export const Heading = ({ heading, level = 2, className }) => { | ||
| const Tag = `h${level}`; // Dynamically chooses h2, h3, etc. based on `level` | ||
| return <Tag className={`heading heading${level} ${className}`}>{heading}</Tag>; | ||
| }; No newline at end of file |
There was a problem hiding this comment.
I like the level approach of your headers! :) Smart!
JennieDalgren
left a comment
JennieDalgren
left a comment
There was a problem hiding this comment.
Great job with the portfolio!
You almost met all requirements but there are a few places where you version doesnt match the provided design.
A designer would not be okay with the images in the header not stacking as they've done in the design. The same with the button and the github icon.
Go through the design file thoroughly and fix the small things to get approved.
…d media queries and fixed buttons styling
|
@JennieDalgren Thank you for your feedback! I have now fixed the issues with the github icon on the button and the stacking of the images in the projects gallery/header. |
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
Hi Anna! I'd like to ask you to take a more thorough look at the design as there are still things that don't adhere. E.g. I found some things specifically in tablet:
- Skills should be centered
- Tech section is too wide
- Featured projects section has too much padding
- My words section has too little padding
Apart from that, ,the tags are not following the design and why the border on the buttons? Remove.
Lastly, check all spacings in all sizes as well as text alignment (center vs left). Example where it's not following the design:
4 participants
Netlify link
https://annahansenportfolio.netlify.app/