
Design comparison
Solution retrospective
Feedbacks are welcomed
Community feedback
- @pikapikamartPosted about 3 years ago
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 helpful0 - Since a
- @byronbyronPosted about 3 years ago
Hi @Mugeez001 Nice work!
I have a suggestion to make your icons look a bit more uniform, by applying the styles to the
.fylo-images span
instead of.fylo-images img
. Might look something like this:.fylo-images span { width: 48px; height: 48px; background-color: hsl(229, 57%, 11%); border-radius: 7px; display: flex; align-items: center; justify-content: center; margin-right: 1rem; } .fylo-images img { }
0@Mugeez001Posted about 3 years ago@byronbyron Thank you so much. your correction was helpful.
0
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