@pikapikamart
Posted
Hey, great work on this one. Layout in desktop looks great, though the mobile layout needs to make use more of the screen-size. Right now, only users with 360px going down can see the mobile layout, phone resolution is much greater than that. I suggest to properly address this, make use of mobile first approach.
Akanksha already said some great tips, just going to add some as well:
- Avoid using
height: 100vh
on large container like thebody
tag, this makes the element's height, uses only the remaining viewport/screen's height. Inspect your layout, hover on thebody
tag, you will notice that it doesn't really capture all the layout. Instead, replace it withmin-height: 100vh
, this takes full height and expands if it needs to. - The purple on the heading tag is missing, check that one out.
- For the card stats, you could have used
ul
element to nest those, since those are "list" if information. - The word
10k
314
and the other one is not suited to be a heading tag. Use heading tag to phrases which gives overview to what is the section is all about, and those words like 10k, does not give any, usep
tag on it. - You don't need to use
figure
on the image, you typically usefigure
when you want an image to be explicitly visible to all, that is why it usesfigcaption
.
Aside from those, great job again on this one.
Marked as helpful
@GColville
Posted
@pikamart
Hey, thank you for the feedback, really appreciate it. I'll go in and make some amendments :)
I was wondering about my use of the <figure> element, would it be better to just have the <img> on it's own there or perhaps create a <div> and set it's background property?
@pikapikamart
Posted
@GColville You could use either of the 2 ways.
But, if you think that the image on the righ side is meaningful enough to the website, you should use img
element with a descriptive alt
.
If the image is not really meaningful and just decorative, then you could use just a div
and make the image just a background-image
, or just use img
element, but the with alt=""
as empty.
@GColville
Posted
@pikamart
I had another go at the challenge and went for the mobile first approach.
For some reason, I can't get the overlay to work for the image, when I do add the background-color to the <div> element containing the image it doesn't work.
I think the rest looks better, but definitely room for improvement.
Thanks again for your advice!