@jairovg
Posted
Hi @fernandanevesf, congrats on your solution; here are some comments that might help you to improve it:
About fonts
- Double check the font link you're using, as it imports
400
as the default weight only. Check this version which is yours, vs this other one which includes all the fonts used in the challenge. - You'd like to add this two more lines, for performance:
<link rel="preconnect" href="https://fonts.googleapis.com">
<link rel="preconnect" href="https://fonts.gstatic.com" crossorigin="">
- Your font families don't have a fallback option, this is a good practice, in case somethinkg happens and the font is not downloaded, i.e:
font-family: 'Montserrat', sans-serif;
.
Accessibility
- I'm wondering why you're using
vw
to define the font size. Consider usingrem
for this, it could be setting the parent font size as100%
to let the user define the default value,16px
by default, and then calculate the font size you want for a particular element. i.e.: If you want an element with14px
by default, then its value would be0.875rem
. - Good choice using
picture
to define the image at different breakpoints. As the card image is a content image, consider adding analt
text to theimg
element so assistive technology can read the image description loud, otherwise, the image would be considered as adecorative
one, injecting an accessibility issue. - You have multiple a11y issues; you can use tools like axe dev tools if you're not using them yet.
Regarding your questions/doubts
...I couldn't make it responsive until I switched to vw
The problems you have here are mainly related to the way you're using flex
in your component and setting sizes using viewport units instead of letting flex-basis
to define the correct size; here is a CSS-tricks blog post related to the use of flex that may help you to solve the issues you have.
...I also had trouble with the image, as it wouldn't fill its half of the container until I set a smaller min-width
Try setting this to the img
element instead of setting its width
and min-width
with viewport units:
img {
max-width: 100%;
height: 100%;
object-fit: cover;
}
These properties set the image to be responsive. Also, check how to define the item to the 50%
of its parent. Check out the blog post above.
Others
- Be careful about abusing the use of viewport units, here is an interesting video about some problems with these units.
- Think about the title structure you have in your component, should
PERFUME
be the main heading? and a subheadingGabrielle Essence Eau De Parfum
? Probably there is not a correct answer and it may sense your structure, but if not, which other way may be used to define the component title?
I hope you find it useful. I'm happy to take another look at your solution if you make some other changes.
Marked as helpful