Design comparison
Solution retrospective
This was really really difficult for me, I think its slightly above my knowledge. I've just begun using flex. Just barely. I just wanted to share this time.
Community feedback
- @correlucasPosted over 2 years ago
👾Hello Jessy! Congratulations for your solution!
I've to say you that you've done a great job for your first time using flexbox. Thats really nice.
I've some tips for you but are small details, the hard work, you've already done and thats fine. You can use flexbox to align the component by the
body
, all you need to do is replace theheight
withmin-heigh: 100vh
to make the component align to its parent (the body). See the changes I did to you code below:} body { display: flex; min-height: 100vh; /* height: 100vh; */ /* margin-left: 300px; */ align-items: center; justify-content: center; }
About the cards, you can replace the
div
with anarticle
to improve the semantics, and use a general class to manage the 3 cards together and on single card to manage the different aspects, like the colors. This way gets easier to change stuff without touch every class separately.Hope it helps and happy coding!
0 - @PhoenixDev22Posted over 2 years ago
Hello Jessie Fabian,
Excellent work! Congratulation on completing this challenge. I have some suggestions regarding your solution if you don't mind:
HTML
- Page should have one level heading. You can add a
<h1>
withclass="sr-only"
(Hidden visually, but present for assistive tech).
- In this challenge , the images are much likely to be decorative. For any decorative images, each img tag should have empty
alt=""
andaria-hidden="true"
attributes to make all web assistive technologies such as screen reader ignore those images .
- What would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the <a>. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.
- You have used
<br>
, using<br>
is not only bad practice, it is problematic for people who navigate with the aid of screen reading technology. Screen readers may announce the presence of the element. This can be a confusing and frustrating experience for the person using the screen reader. You can read more in MDN
- It’s not recommended to use
<br>
to increase the gap between lines of text Or make empty space between elements, usepadding / margin
styling via CSS. And about using <br> in the<p>
to make the paragraph break in new line, You may usemax-width
to<p>
and remove those<br>
There are so many repeated style rules , better to use reusable and manageable classes. For example: each column have the same styles , So you can use a class .card for the shared styles, then for each distinct styles like (background color) use other classes . card__sedans, . card__suvs and .card__lux
line-height: 25px;
Use a unitless line-height value to Avoid unexpected results. You can read more in mdn
- Check the component responsiveness again.
Overall, Your solution is great. Hopefully this feedback helps.
0 - Page should have one level heading. You can add a
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord