React Portfolio- Bianka Romero #19
Conversation
govargas
left a comment
govargas
left a comment
There was a problem hiding this comment.
Well done Bianka! Looks super nice and 1:1 to the original! I got some comments here and there, but overall it looks really clean and follows the requirements nicely. Keep it up! 🙌
src/components/StyledTags.jsx
Outdated
| white-space: nowrap; | ||
| @media ${media.tablet}, ${media.desktop} { | ||
| flex: 1 0 0; |
There was a problem hiding this comment.
Small detail I notice, on mobile (320px) the pills overflow. Maybe adding flex-wrap: wrap; row-gap: 8px; can help so extra tags wrap to new lines with consistent vertical spacing
There was a problem hiding this comment.
That's weird bc it doesn't show up overflowed on chrome for me. I actually spent a TON of time fighting with the responsiveness for each section lol.. Could you tell me what browser you're seeing that on?
There was a problem hiding this comment.
My mistake, it isn’t overflowing. What I meant was that when the tags wrap, there isn’t any vertical gap between the rows as in the Figma file, so they sit flush together. I double checked across a few screen sizes and it looks like you intentionally kept that consistent, which totally makes sense and keeps everything uniform anyway 👍
| <meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
| <title>Portfolio</title> | ||
| <title>Dev Portfolio - BLR</title> |
There was a problem hiding this comment.
Quick fix for higher SEO score in Lighthouse: add <meta name="description"…> with like one sentence summarizing your site
| ` | ||
|
|
||
| const Card = ({ title, text, tags, TitleComponent = CardTitle }) => { |
There was a problem hiding this comment.
I really like how you combine the title/body/tags logic into one reusable component here in Card.jsx. It’s very DRY. 👍
There was a problem hiding this comment.
For screen-reader users you could add an alt or aria-label on each wrapper
| justify-content: center; | ||
| gap: 24px; | ||
| @media (min-width: 426px) { |
There was a problem hiding this comment.
Got a bit confused, why is this media query here and not in media.js? Just curious 🙂
There was a problem hiding this comment.
lol yeah super random, bc I was fighting so much with everything formatting in a px perfect way- I decided the mobile view format for this section only looked good up to this point (which is technically still considered mobile) but I wanted the centered tablet format to kick on from here so I just inserted the specific px breakpoint. Since I only use it once, I didnt add it to media.js
HIPPIEKICK
left a comment
HIPPIEKICK
left a comment
There was a problem hiding this comment.
A11y
- Requirement not met: You should have a score of at least 95 in Lighthouse
From Lighthouse:
- Links do not have a discernible name
- Heading elements are not in a sequentially-descending order
Content
- Missing: A presentation of some thoughts that you have around code. (If you didn't write an article last week - use placeholder text for now)
Regarding the requirement: Your portfolio should follow the Figma design
Really good job following the design Bianka! The only small things I see are:
- The Skills should be left-aligned in desktop
- I think it would look good if the header and footer also got 128px top/bottom padding
- Some margin between project tags
Codewise
Nice job structuring your project and utilising components and props. You’ve taken inspiration from the structure we discussed on the live sessions - I like it 👍
Changes requested
- A11y
- Missing content
|
Accessibility score should be at least 95 in Lighthouse. Almost there! |
4 participants
Please include a link to your Figma design and a Netlify link.
Figma: https://www.figma.com/design/hFjGb9pVuXS6SbxUAGdCkL/--Bianka---Figma-designs-for-students--Copy-?node-id=3084-1056&m=dev
Netlify: https://biankaromero.netlify.app