@pikapikamart
Posted
Hey, awesome work on this one. The desktop view looks great, the site is responsive but at mobile state, at around 340px upwards, the site's contents are being hidden by the screen because the 1000gb text part is not responding well to the site's changes.
Byron already gave a helpful feedback on this one, just going to add some suggestion as well:
- Since a
main
tag is needed for every page to distinguish the main-content of the site, replace thesection
to usingmain
on the .container
selector. - Remove the
height: 100vh
on the .container
. Avoid usingheight: 100vh
on a large container 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. - Instead of using the
max-width
on the.container
use it on thebody
tag, this way, even if you zoom out on your screen, the layout will stay consistent or maybe you can just addbackground-size: 100%
on the.container
. - Remember that a website-logo is one of the meaningful images on a site so use proper
alt
for it. Use the website's name as the value likealt="fylo"
. - Also, when using
img
tag, you don't need to add words that relates to "graphic" such as "logo, image" and others, sinceimg
is already an image so no need to describe it as one. - Those 3 icons below the site-logo are just decorative
img
tags. Decorative images are just images that doesn't contribute to the overall content of the site. They should be hidden for screen-reader at all times by usingalt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if you are usingsvg
instead ofimg
tag. - Also, it would be nice to change the
span
that holds the each of the icon to using abutton
since it looks like the component is interactive or usediv
. - On a site, always have a single
h1
. Since there are no visible text that are suitable to beh1
, theh1
would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, theh1
would have likesr-only
class and the text-content should describe what is the main-content is all about. Theh1
could be placed as the first text inside themain
.Have a look at this simple snippet of mine using sr-only h1 comments are already included in there, but if you have any queries just let me know. - When wrapping up a text-content, make sure that it is inside a meaningful element like
p
tag or heading tag and not using likediv, span, strong
to wrap the text.
MOBILE STATE
- Don't use
width: 100vw
since this will only add a horizontal scrollbar at the bottom, since this value does not account the vertical scrollbar's width. - Adjust the
.progress-bar
so that the site will properly resize and won't create the horizontal scrollbar since the content will be prevent to overflow.
Aside from those, great job again on this one.
Marked as helpful
@Mugeez001
Posted
@pikapikamart Thanks for the correction.