@vanzasetia
Posted
π Hi Angel!
π Congratulations on finishing this challenge! I have some feedback on this solution:
- Accessibility
- All page content should live inside a landmark (
header
,main
, andfooter
). In this case, you can wrap all the page content withmain
tag. - For any decorative images, each
img
tag should have emptyalt=""
andaria-hidden="true"
attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only, except the photos. - For the testimonial photos, I recommend using their names as the alternative text.
- To learn more about how to make images accessible, you read the tutorial from W3 WAI about images and the ultimate guide for alternative text by Axess Lab.
- Heading tags should always be in order. Also, there's no need to make the reviewer's name as a heading, since the content below it, is too small.
- You can make the
stats
to be aul
instead of asection
and wrap each list item withli
.section
andarticle
always needs a heading as a label. - Wrap the testimonial with
blockquote
for semantic HTML.
- All page content should live inside a landmark (
<blockquote>
<p class="card__text">
"We needed the same printed design as the one we had ordered a week prior.
Not only did they find the original order, but we also received it in time.
Excellent!"
</p>
</blockquote>
- Use
rem
or sometimesem
unit instead ofpx
. Usingpx
will not allow the users to control the size of the page based on their needs. - Styling
- For the background images, I would recommend using
background
properties on thebody
element. - I would recommend making the
body
element as a flexbox container to make the page content perfectly in the middle of the page.
- For the background images, I would recommend using
body {
display: flex;
align-items: center;
justify-content: center;
min-height: 100vh;
}
- There's no need to set a
min-width: 100vw
since by default thebody
element already have100%
width. - I would recommend using the mobile-first approach when you write the styling. It often makes me write less code. Also, you can avoid having complex media query, especially on this project. So, all you have is the
@media screen and (min-width: 80em)
. (80em
is equal to1280px
). - Also, in addition to
@media
query, I recommend usingem
unit instead ofpx
. You can read this article about what unit you should use on@media
query.
That's it! Hopefully, this is helpful!
Marked as helpful