@pikapikamart
Posted
Hey, great work on this one. Desktop layout looks fine, just needed for the background-color of the site a bit dim and font-sizes for me are too little. Mobile state looks fine, but resizing it first, the layout is very squished, you could made something like a 2x2 column before going into mobile view so that the layout won't look squished and transition properly.
Ken already gave great feedback on this one, just going to add some suggestions as well:
- Avoid using
vh
unit as thefont-size
value, never use it. It might look good on your end but users who have smaller screen-size-height will get different sizings. Userem
values instead to make it more consistent. - Avoid using
height: 100%
orheight: 100vh
on a large container like thebody
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to.
Okay ugm, removing the height
the layout has just been destroyed. Looking at it, you almost use position: absolute
to each element on the site. Why? Using position: absolute
removes an element from the flow and must be used only when you are sure that you control the element properly. On this one, it is not idea to or in general, use position: absolute
on each element. It would be great to have another go with this challenge and complete it without using position: absolute
.
But still, here are some continuation of my suggestions, but I won't include css for now, those are on your part:>
- You should only use a single
h1
to wrap those text above. That is just a single phrase so only use 1h1
to wrap those text. Usemax-width
to make the text wrap, adjust it until you get the desired look. - Avoid using
id
to target and style an element since it is a bad practice due to css specificity. Instead, just useclass
to target element. - Each of the card titles should be using
h2
and noth3
. When using a heading tag, make sure you are not skipping a level. If you useh3
then make sure thath1, h2
are all present. - Those card icons are only a decoration on the site. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if the image is usingsvg
. - Also, only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
Those only for now, but as I suggested, it would be great to have another go with this one with proper css usage.
But still, great job for this one.
- When using
img
tag, you don't need to add words that relates to "graphic" such as "logo" and others, sinceimg
is already an image so no need to describe it as one.
Marked as helpful
@Abdul400
Posted
@pikamart thank you so much for your feedback. Your feedback is very in-depth and I really appreciate it. I will definitely redo the CSS code by incorporating your advice and also that of Ken. Thank you so much :)