Hi
I'm afraid the HTML needs to change on this
The biggest issue is with those 3 stats. Numbers like that make no sense at all as headings - what kind of content would you expect to find navigating to a title of "10k"? It's meaningless. Imagine the content in a plain document without styles. That helps with choosing the most appropriate elements. Instead those 3 stats should be a list with 3 list items
This is a single component with text and an image. If you're going to use section, it should be wrapping all that content, including the image as it is all one component.
A more minor issue is it is generally considered bad practice to have empty divs in html. Not only is it confusing to other developers (why is that div there) but it is also less performant serving background images like this. Instead, use the picture element. Alt can be blank to treat it as decorative, as you are doing now, but the code is clearer and the performance is way better because the browser can do it's job and pull down the correct image for the users screen
Marked as helpful
Css looks OK but I am concerned at the number of % widths in this. I think you may have a misunderstanding about paddings vs margin.
Margin is for external space. For example creating vertical space between low level elements in the card like paragraph and list. Padding is for internal spacing. For example creating some space inside the card content half of the component so nothing hits the edges of that box
Marked as helpful
@walkonmars36
Posted
@grace-snow
Hi Grace, thank you so much for your review and feedback.
I have changed the stats content to be an <ul> - makes total sense 🙄
As this is just a single component that would most likely be a section on a bigger page, through trying to be semantic I wasn't sure what to use to avoid everything being a div. I have opted to now just using <main> to wrap everything.
I deliberated for so long on what to do with the image, I took on board what you said about empty divs and tried using the <picture> tag, but this has <img> as a requirement. So I went back to the research. I believe that this is a purely decorative image so what I decided on was to use it as a pseudo element, with a comment in the HTML and CSS for good developer practice. I'll reply to your other points below
@walkonmars36
Posted
@grace-snow
Hello again 😁
Point taken on my excessive use of % widths, I've gone in and tidied that up, using padding and also removed some margin that was coming through from the pre media query styles.
A couple of new ideas I had was to use clamp on the padding-right of the stats container, as the items were getting squished at a certain point before snapping back to mobile layout.
As I'd been using one of those % widths to control the text section, I applied a max-width of 40ch to this. I learnt this tip from a recent blog post from Andy Bell.
Thanks again for your advice, I've just run my site through Lighthouse and got 100% for accessibility and best practices 🎉
@walkonmars36 this looks really good, well done!
@walkonmars36
Posted
@grace-snow
Thanks Grace 😊
