VariaSlu portfolio PR #28
Conversation
ssofiejohansson
left a comment
ssofiejohansson
left a comment
There was a problem hiding this comment.
I think you have done a great job, Varia! I know you felt you fell a bit behind due to illness but you def caught up in the end. Code looks clean, don't have much to add. Some minor things I have pointed out in comments below.
The design is simple but effective. I hope you do implement the animation on the eyes following the curser as you mentioned in the beginning, that would make the page pop for sure!
After doing a Lighthouse rapport, I think some quick fixes could be to convert your images to webp, and check the contrast on the Communication heading under Skills.
All and all, be pround and happy with your work. Well done!!!!!!!
| <Header /> | ||
| <Tech /> | ||
| <FeaturedProjects /> | ||
| <Skills /> | ||
| <Contacts /> | ||
|
|
There was a problem hiding this comment.
Thanks, I like it too. Anyway, I would structure the files differently if I did it again.
src/components/ProjectCard.jsx
Outdated
| <Button href={project.links.process} className="read-article"> | ||
| <FaFileAlt /> The process |
There was a problem hiding this comment.
Excited to see what will be included in this process button!
src/data/projects.json
Outdated
| @@ -1,5 +1,18 @@ | |||
| { | |||
There was a problem hiding this comment.
Is this json neccessary? Maybe remove if not used.
There was a problem hiding this comment.
You're right, this should go away
| body { | ||
| background: pink; | ||
| color: hotpink; | ||
| } |
There was a problem hiding this comment.
Maybe add a margin and padding: 0 to remove the white "border" from your different sections
There was a problem hiding this comment.
Such a great tip, you're for sure a padding queen
| const Title = styled.h2` | ||
| color: white; | ||
| font-size: clamp(32px, 6vw, 80px); | ||
| font-family: Montserrat; | ||
| font-weight: 700; | ||
| word-wrap: break-word | ||
| `; | ||
|
|
||
| const Columns = styled.div` | ||
| display: flex; | ||
| justify-content: space-around; | ||
| flex-wrap: wrap; | ||
| gap: 2rem; | ||
| max-width: 1200px; | ||
| margin: 0 auto; | ||
| `; |
There was a problem hiding this comment.
The headings (specifically Communication) overlaps with More when scaling down the page. (see around 980px width on the page) Maybe use justify-content: space-between on the Columns div instead to avoid this.
There was a problem hiding this comment.
Great catch and suggestions! I will solve by changing the copy for now (to Personality) :)
| export const Contacts = () => { | ||
| return ( | ||
| <Section> | ||
| <Title>Contact (But No Pressure)</Title> |
| Creative <Highlight>+</Highlight> Developer | ||
| </Title> | ||
| <IntroContainer> | ||
| <Face src={faceIcon} alt="face emoji" /> |
There was a problem hiding this comment.
I remember you said you want this face icon animated so the eyes follow the cursor (or something like that) and I think you should try to implement it cause it'd look supercool!
There was a problem hiding this comment.
Thank you for the encouragement, I will do
| font-size: clamp(32px, 6vw, 80px); | ||
| font-family: Montserrat; | ||
| font-weight: 700; | ||
| word-wrap: break-word; |
There was a problem hiding this comment.
I have never used this, so asked chatgpt to explain what it does. Apparantly now you should use overflow-wrap: break-word; instead, as word-wrap is an older way of using it.
There was a problem hiding this comment.
Ah, I see. Okay, now I know too
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
A11y
Background and foreground colors do not have a sufficient contrast ratio.
Your code looks good, you’ve understood the point of components and props. Would love to see you challenge yourself more going forward and think even more about reusability.
Regarding the requirement: Your portfolio should follow the Figma design
Make sure all your section has a somewhat consistent top/bottom spacing. The Tech section has a little to little according to the design and the Skills have more top than bottom padding etc… Double check all spacings and update your page according to the design please.
Changes requested
- A11y (contrast)
- Check the design again and follow more closely
|
Hi Matilda, Regarding a sufficient contrast ratio, I assumed it might be Tech or Skills, but they pass Silktide. Could you please point out which ones you meant need to be updated? Thank you! |
|
Yes, sure - this is what Lighthouse gives me: 
|
|
Ah, thank you! I've updated it now, it should be fine. |
3 participants
Please include a link to your Figma design and a Netlify link.
https://www.figma.com/proto/ckfCpTXZdALIKnWf0PGSHp/Figma-designs-for-students--Varia-?node-id=1078-1624&t=5Jgpgvf8oWIpGrpz-0&scaling=min-zoom&content-scaling=fixed&page-id=1078%3A906
https://variaslu.netlify.app/
Hello, by the end I've realized that that design version was the most complex :)