Skip to content
Submitted about 3 years ago

four-card-feature-section-master

omar farid•370
@omarfarids
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Four card feature section
  • HTML
  • CSS
1newbie

Design comparison

SolutionDesign

Solution retrospective

any comments on my work or any adjustment I need to do ?

Community feedback

  • Raymart Pamplona•16,100
    @pikapikamartPosted about 3 years ago

    Hey, awesome work on this one. Desktop layout looks really nice, though when resizing the screen, the layout is being hidden thus creating horizontal scroll. It would be great to make the site more responsive. Mobile state looks great but your breakpoint of 375px is too little, other phones have higher width than that so they won't get the mobile layout, toning it up would be really great and also, using mobile first approach will be really great since it will help you addressed issues like this.

    Ken already gave great feedback on this one, just going to add some other suggestions as well:

    • The text Reliable, efficient delivery Powered by Technology should be using a single h1 on this one, since that is just a single phrase with the second part on the next row. Use h1 to wrap both and use max-width on it so that the text will wrap like on the design, adjust it until you get the desired look.
    • When using a heading tag, make sure you are not skipping a level. If you use h4 then make sure that h1, h2, h3 are all present. Changing those other heading tags into using h2 would be great.
    • Those 4 icons being are only decorative. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if the image is using svg.
    • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
    • Lastly, making the site as responsive as you can so that you can avoid the issue when resizing the screen.

    Aside from those, great job again on this one.

    Marked as helpful
    0
    omar farid•370
    @omarfaridsPosted about 3 years ago

    @pikamart hey, thank you for this great feedback. great points you have mentioned. and I would ask you about some of them : -making horizontal scroll by (overflow-x:scroll) ? -when using (alt=) I write discription for the Image not statement that it can't display? -what (aria-hidden="true") does ? -when I get frontend mentor feed back ,it gives me something about accessibility issue (<html lang="en">) and don't really know how to fix this. again thank you for your feedback. I'm new at this field and it really helps.

    0
    Raymart Pamplona•16,100
    @pikapikamartPosted about 3 years ago

    @omarfarids Hey!

    • For the horizontal scroll, like what I said, the layout is not responsive enough so if you go for example at like size 600px, you will see that the layout is being hidden and the horizontal scrollbar is at the bottom which means layout is not responding well.
    • Yes, text will not be visible when using alt but, only use descriptive text or descriptive alt when you think that an image adds content to your site. If the image is just purely decoration, hide them using:
    <img alt="" aria-hidden="true" />
    • aria-hidden="true" makes an element not be caught by screen-reader. You typically do this when there is a content like a decorative image on the site that doesn't really add any meaning, you use the attribute so that the img will be hidden.
    • For the last issue, as you may have read it, page should have level one heading tag or an h1. Like what I suggested at my previous feedback, use the h1 to wrap the two bold text on the site. This way, your site will have an h1.
    Marked as helpful
    0
    omar farid•370
    @omarfaridsPosted about 3 years ago

    @pikamart thanks for all these details and I will be grateful if I always get your feedback on my work ..

    1
  • Ken•935
    @kenreibmanPosted about 3 years ago

    I would get rid of margin: 50px auto; styling on your main element. Instead I suggest doing:

    body {
  min-height: 100vh;
  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
}

    which will center your content.

    I would also give more of a gap between your containers in desktop view.

    Other than that, your mobile layout looks great! I hope this helps.

    Marked as helpful
    0
    omar farid•370
    @omarfaridsPosted about 3 years ago

    @lmaoken that is so helpful thanks I will take care of these points and also use flex centering more in the future rather than margin auto

    0

Please log in to post a comment

Log in with GitHub

Join 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