Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Log in with GitHub
OverviewSolutions6Comments5

All comments

  • Murali• 10

    @Murali3753

    Submitted

    hello guys give me some feedback about my challenge

    HTML ,CSS

    1

    Chow Jia Ying• 135

    @C-likethis123

    Posted

    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• 135

    @C-likethis123

    Posted

    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• 135

    @C-likethis123

    Posted

    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• 135

    @C-likethis123

    Posted

    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• 135

    @C-likethis123

    Posted

    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