Skip to content
OverviewSolutions
7
Comments
5
Chow Jia Ying
@C-likethis123

All comments

  • Murali•10
    @Murali3753Submitted over 3 years ago

    hello guys give me some feedback about my challenge

    HTML ,CSS

    1
    Chow Jia Ying•145
    @C-likethis123Posted over 3 years ago

    As a whole, I like the code. It's quite neat and I got to learn about flex-wrap and transform.

    However, I would not recommend setting push as an element ID because it is used for more than one element. IDs are supposed to be for a specific element. I suggest you create it as a class.

    Other than that, I have no comments and I'm actually quite impressed by your solution.

    0
  • Chow Jia Ying•145
    @C-likethis123Posted over 3 years ago

    Some comments:

    • Instead of naming your divs 'module1', 'module2', try to have more descriptive names for your HTML elements. For example, you can name them "text-content" or "features". CSS shorthands:
    • For lines 104-107 of index.css, you can specify two values instead of four values: #module1 { padding: 0px 270px; } This has the same effect as your current code, but with less values specified.
    • Same thing for line 98, which can be simplified to margin: 2em;
    • For more information, google about CSS shorthand syntax for paddings/margins.

    Font sizes:

    • I personally found 1.3em too large on a screen size of 1440px, and preferred 1em instead.

    Responsive design:

    • The website switches to mobile layout on screen sizes less than 1440px, which to me is too large a breakpoint. There are laptops that have a screen size of >1000px, so the mobile layout looks too stretched out on these screen sizes. I recommend to google the common breakpoints and experiment with a suitable breakpoint to switch to mobile layout.
    1
  • Chow Jia Ying•145
    @C-likethis123Posted over 3 years ago

    There was a good attempt to make the design responsive for different screen sizes. Some issues I have noticed:

    • On smaller screen sizes, the text is supposed to align to the center, but the button continues to be aligned to the left even after there was a CSS rule to make the text align to the center. Instead of: h1, p, a { text-align: center; }

    Try: #content { text-align: center; }

    • From screen sizes 390px to 950px, the background image doesn't exactly align with the edges of the screen. Try adding background-size: contain to fix that.
    0
  • Chow Jia Ying•145
    @C-likethis123Posted over 3 years ago

    Why your h2 elements don't align with the secondary p elements:

    • It's because of the rule:
    @media (min-width: 768px){
    .information p:first-of-type{
        margin: 5% 15% 2% 10%;
    }
}

    I think you intended for this rule to target the first <p> element in the <div class="information"> element, but it ended up targeting all the p elements in that element.

    There are two ways about it:

    • Assign an ID to the <p> element, and apply that rule based on the ID selector. Eg I would change it to <p id="description"> Then my rule will be changed to
    @media (min-width: 768px){
    .#description {
        margin: 5% 15% 2% 10%;
    }
}
    • Another way is to override the margins in the <p class="upper"> elements. Originally p.upper{} alone was not specific enough (refer to CSS selector specificity), so I had to specify:
    @media (min-width: 768px){
    div.data_text p.upper {
        margin: unset;
    }
}

    Onto other comments:

    • I like how your code is mobile first and the fact that you used CSS resets.
    • However on some screen sizes (around 900px), the statistics overflowed out of the container, so you might need to consider these screen sizes as well.
    • On smaller screen sizes, it looks nicer if there was lesser horizontal padding so there is more space for text, eg:
    information {
    padding: 12% 5%;
    flex: 2;
}

    instead of just padding 12%;

    1
  • Chow Jia Ying•145
    @C-likethis123Posted over 3 years ago

    In general, I like your code organisation and how clean your code is.

    Some things I have noticed:

    • On screen sizes below 830px, some of the text tends to overflow out of the container. Instead of setting a fixed height, you could set it to height: fit-content.
    • The size of the statistics in your solution seems to be smaller than the original design
    • You have some code that specifies two font weights like this: font-weight: 400, 700. I'm not sure whether it works on your side, but in my browser it's flagged as an invalid property value.
    • The words (particularly the description) in your solution seems to be less spaced out than in the original design. You can add a line-height: 20px to increase the spacing between lines.
    1