SaraEnderborg-js-project-portfolio#45
Conversation
Added links to Netlify and Figma in the README.
| gap: ${({ theme }) => theme.buttons.gap}; | ||
| height: ${({ theme }) => theme.buttons.height}; | ||
| padding: 0 ${({ theme }) => theme.buttons.paddingX}; | ||
| border-radius: ${({ theme }) => theme.buttons.radius}; |
There was a problem hiding this comment.
Nice! To have everything from border radius etc. defined from the theme file!
| @@ -0,0 +1,112 @@ | |||
| import styled from "styled-components"; | |||
|
|
|||
| export const Container = styled.section` | |||
There was a problem hiding this comment.
Would maybe add a gap property here (e.g. gap: 32px) to get some spacing between the text and the image :)
| justify-content: center; | ||
| } | ||
| `; | ||
| export const ProfileImg = styled.img` |
There was a problem hiding this comment.
I think a height property of 480px or similar would be nice here - IF you are aspiring to create a round image like the Figma design so to say :) Another tip is to export the whole frame (image with the border) from Figma and insert it instead of styling it with CSS here, if you prefer that!
| @@ -0,0 +1,28 @@ | |||
| import styled from "styled-components"; | |||
|
|
|||
| export const Card = styled.article` | |||
There was a problem hiding this comment.
Might be easier to name is StyledCard directly here so you don't have to rename it in the import in Card.jsx :)
| @@ -0,0 +1,36 @@ | |||
| import Button from "./Button"; | |||
| import { Card as StyledCard } from "./Card.styles"; | |||
There was a problem hiding this comment.
Looks really clean! A small tip is to maybe add a main-tag, and/or change some sections to to instead be header- and footer elements to improve accessibility scoring on Lighthouse :-)
There was a problem hiding this comment.
Nice! I like the use of theme and that you also include things like button layout as well!
src/components/Card.jsx
Outdated
|
|
||
| <div style={{ display: "flex", gap: "1rem", flexWrap: "wrap" }}> | ||
| {demo && ( | ||
| <Button href={demo} $variant="secondary"> |
There was a problem hiding this comment.
The buttons seems to pick up the same styling even if primary and secondary button coloring is defined in theme, but I assume it's still on the to-do list :)
gabriellaberko
left a comment
gabriellaberko
left a comment
There was a problem hiding this comment.
Well done with your project, Sara! You should be really proud to have come so far with React! I think you managed to create a really good structure in the project and I like how you have organized your files, and made use of both styled components, theme and global styling as well in a good way to make styling changes more efficient in the future.
I know that the website is still a work in progress when I'm reviewing this - but the website already looks really good, is responsive and follows the design. There are still some parts that needs to be looked over, such as the card button styling to fully match the design, and a little bit of accessibility work to score higher on Lighthouse. But you are almost there! Again, really great work with this!
| } | ||
|
|
||
| .swiper-slide { | ||
| width: 400px !important; |
There was a problem hiding this comment.
Using !important is a last resort. I guess you, in this case, want to override the css from swiper?
Duplicating the classname increases the specificity enough to overwrite it:
.swiper-slide.swiper-slide {
width: 400px;
...
}
| font-weight: 500; | ||
| color: #202020; | ||
| margin: 0; | ||
| font-size: clamp(18px, 2vw, 22px); |
There was a problem hiding this comment.
Nice! using clamp for font sizes is a really nice way of making it smooth.
Npahlfer
left a comment
Npahlfer
left a comment
There was a problem hiding this comment.
Great job Sara!
Really easy to follow along and find everything - good structure and naming.
The only thing I would have wanted to see more consistency in, is the use of the theme. You sometimes use it and other times it's hard coded.
But overall really good!
3 participants
Figma: https://www.figma.com/design/er7xor6mcZzPVT8ZM0tY0w/Portfolio-designs?node-id=10817-954&t=SPZgu2bO284eVn9U-0
Netlify:https://portfolio-saraenderborg.netlify.app/