Skip to content
OverviewSolutions
11
Comments
5
Suleyman
@Suleeyman

All comments

  • Suleyman•360
    @SuleeymanPosted over 3 years ago

    The layout is well done, except your image between [900px - 800px]. To remain for this issue, you can simply add a height: 100% on your img__container. Then add height: 100% on your images and, object-fit: cover;

    Also, you should add a padding-top and -bottom on your flex__container--content. Moreover, you don't need a neested <div> on your flex__container--content. And for finish, in SCSS, variables are declared like:

    $my-variable: value; And you don't need to nest them in your :root element.

    In SCSS we can nest our CSS style like: header { h1 { } } Which means in CSS: header h1 { }

    0
  • aboalfadel1•270
    @aboalfadel1Submitted over 3 years ago

    I don’t know why the property "align-items" did not work in the body here to center the main div. I used the margin instead. if someone knew why, it would be nice to tell me

    3-column preview card component

    1
    Suleyman•360
    @SuleeymanPosted over 3 years ago

    Well, it actually works, but your body isn't taking more height than the div itselfs. So your body's height is equal to your div's height.

    About your layout, instead of display: grid and grid-template, you can simply use display: flex; add it a flex-wrap: wrap, and add a width of 300px on his childrens.

    1
  • Suleyman•360
    @SuleeymanPosted over 3 years ago

    Your design is well done, congratulations. But, in your HTML code, instead of using div element, you can use semantic tags (header, section, footer, main,...) It's better for SEO.

    3
  • Suleyman•360
    @SuleeymanPosted over 3 years ago

    First of all, you don't need to toggle the icon-share on your js script.

    Also, don't use div tag, when you can use semantic tags like header, nav, section, main, footer,... (they are all display: block) It's better for SEO, read more about it: https://www.freecodecamp.org/news/semantic-html5-elements/

    Your media querie is a mistake: @media screen and (max-width: 1500px){ .container { width: 95% } }

    max-width: 1500px means this interval [1500px; 0px]. Then your container destroy the image and the layout.

    0
  • Suleyman•360
    @SuleeymanPosted over 3 years ago

    First of all, I would say that, don't use id attribute in common tag like <p> <h1>etc... Also, you are using sec value on your images (max-width: 45rem). Use width: 100% instead. Then the image will take 100% of his main parent.

    Moreover, your are centering your icons with padding-left: 45%. Well you can simply add text-align: center on div.

    And finally: why you use grid everywhere ? When the layout is from top to bottom (in this case in mobile design all items are from top to bottom) you don't have to. I think you have to review the basics, like

    display: block; (div, header, main, nav, footer,... tags are all in display: blocks. Which means that they take 100% width of the parent element.),

    display: inline-block,

    display:inline ( "a" tags are inline, "span" also...)

    0