Design comparison
Solution retrospective
I'm still in the process of learning CSS. Please leave any suggestions for me to make it better !
Community feedback
- @PhoenixDev22Posted almost 3 years ago
Hello @bmhuyquoc104 ,
I have some suggestions regarding your solution:
-
First of all, there isn't any gap in the mobile layout.(remove
gap: 2em;
). -
you can add a
<h1>
withclass="sr-only"
(Hidden visually, but present for assistive tech). Then use<h2>
instead of<h1>
.
.sr-only { border: 0 !important; clip: rect(1px, 1px, 1px, 1px) !important; -webkit-clip-path: inset(50%) !important; clip-path: inset(50%) !important; height: 1px !important; margin: -1px !important; overflow: hidden !important; padding: 0 !important; position: absolute !important; width: 1px !important; white-space: nowrap !important; }
-
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. In this case, all images are decorative only. -
don't use divs for meaningful content. Clicking those "learn more" buttons would trigger navigation, so the right interactive element is
<a>
<a href="#" class="more" aria-label="" >Learn More</a>
CSS
-
border-radius
andoverflow hidden
to the container that wraps the three cards.so you don't have to set it to individual corners. -
A hover effect that raises a button UP looks strange. It's not a natural movement to happen. you can
border: 1px solid transparent ;
to the <a> before the hover and on the hover it woud beborder: 1px solid hsl(0, 0%, 95%);
Your github repo is not accessible (not found).
Last , I would suggest to read more about clamp function
Overall , your solution is good . Hopefully this feedback helps
Marked as helpful0@bmhuyquoc104Posted almost 3 years ago@PhoenixDev22 Thank you so much for your help ! I will follow your advices to make my it looks better!.
1 -
- Account deleted
Hello there! 👋
Congratulations on finishing your challenge! 🎉
I have some feedback on this solution:
put the .card div inside <section> tag to be more setmantic
i hope this is helpful
Marked as helpful0
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