Design comparison
Solution retrospective
The overlay background was a challenge, I learned about background-blend-mode property.
Any feedback is welcome !
Thank you in advance.
Community feedback
- @srikargunnamPosted over 2 years ago
Hi Alexandro Munera,
First I would like to congratulate you for completing this challenge 🎉
I would like to give few suggestions based on my observations.
-
It is must to use h1 tag (level one heading) as per web accessibility standards, you can swap h2 with h1, and my advice is to use a header tag instead of a div tag to wrap h1 and p tags.
-
Your card looks stretched, that is because you used width as 90%, so there is no max-width, Instead of using width as 90% use min(90%, "the max width the content fits good"), so that your content looks good even or much wider screens.
-
Breakpoint for mobile screens mentioned in the style guide is 375px, but used a breakpoint at 768px, it would be better if you can work on that, i can understand that the content looks like weird when it reached 768px, for this you could as a media query for tablet assuming it is given in this project, it will give you a new opportunity to work on your own ideas to make the content suit tablet view until it reach 375px.
-
I have seen that you have used rem units at some places and px units at some places, it is a good practice to stick to one type of units, i would suggest to go with rem entirely.
This link "https://www.sitepoint.com/understanding-and-using-rem-units-in-css" will help you get a better understanding of rem units, and how you can easily use a trick to make 10px = 1rem, this will help you easy to calculate and use rem units, but keep in mind that this trick will not work for media queries which is not mentioned in the above link.
hope this helps 😉, please mark as helpful if it does.
Marked as helpful1@AlexandroMuneraPosted over 2 years ago@srikargunnam Thank you so much, exelent that trick to work with 10px = 1rem and the function min().
0@srikargunnamPosted over 2 years agoHi @AlexandroMunera, I forgot to mention that the trick with 10px = 1rem works only for font-size, sorry if i misguided you by not mentioning this.
Happy coding 😉
1 -
- @PhoenixDev22Posted over 2 years ago
Hello @AlexandroMunera,
I have some suggestions regarding your solution :
- Page should contain a level-one heading , so you can add a
<h1>
withclass="sr-only"
(Hidden visually, but present for assistive tech).
.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; }
-
Never use
<span> '
s or<div>'s
for meaningful content. -
For
class="card__stats-list"
, it would be better if you use an unordered list<ul>
with 3 items for the stats. -
The number and word have to be read together to make sense so need to be in the same meaningful element. so only a
span
or maybestrong
tag needs to wrap the numbers. the words likecompanies
should not be in paragraph tags. They don't need to be wrapped in anything as they are already inside a meaningful element (list item)
Overall , your solution is good, Hopefully this feedback helps.
Marked as helpful1@AlexandroMuneraPosted over 2 years ago@PhoenixDev22 Thanks Phoenix, I understood the importance of h1 tag and the use of ul tag.
1 - Page should contain a level-one heading , so 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