Skip to content
OverviewSolutions
8
Comments
89

Amal Karim ✅

@amalkarimIndonesia1,290 points

CSS is interesting

I’m currently learning...

How to live a peaceful and meaningful life

Latest solutions

View more solutions

Latest comments

  • Amal Karim ✅•1,290
    @amalkarim

    Hi Enn Ko, congratulations for completing the challenge!

    Please check at this line:

    <img src="./images/image-hero-mobile.png" class="w-100" style="height: 450px;" alt="">

    I think it's better that you only set its width using class="w-100", or only set its height using height: 450px;. Don't use them both, because it will make the image not proportional in mobile view.

    We could also add alt for the hero image because it's an important image and adding alt will make it more accessible to all users. For example:

    <img src="./images/image-hero-mobile.png" class="w-100" alt="A curly hair man standing with a laptop on his hand in front of a yellow wall">

    Please refer to this article from W3Schools about image accessibility for more explanation.

    Hope this helps. Happy coding!

    Marked as helpful
  • Amal Karim ✅•1,290
    @amalkarim

    Hi Ted!

    When I visit your solution page using a smartphone in landscape mode, some of the content are hidden and I can't scroll down to see them. I believe this is not because of overflow: hidden; of the section element, but rather because of this style:

    html, body {
    height: 100%;
}

    When the height of the browser is not enough to show all of its content, some of the content become hidden, because the browser height is not allowed to grow taller than 100%. It can be solved by changing height: 100%; to min-height: 100%; or min-height: 100vh;.

    Other than that, I think it's quite a nice solution. But if you want to take the extra step, here is what I suggest:

    That's it. Hope this helps. Happy coding!

    Marked as helpful
  • Amal Karim ✅•1,290
    @amalkarim

    Hi Dennis,

    How do I keep the button highlighted after clicking?

    You used <button> element for the number button. Button doesn't have on and off state, so it's difficult to make the button highlighted after clicking. Though I think it could be achieved by javascript, I'll consider it as hacking. The best element you could use for that is <radio>. It has on and off state (checked or unchecked), so when the user click a number, it becomes checked, and you can give it the style that you want.

    Here's an example.

    <input type="radio" id="rate1" name="rate" value="1">
<label for="rate1">1</label>
<input type="radio" id="rate2" name="rate" value="2">
<label for="rate2">2</label>
<input type="radio" id="rate3" name="rate" value="3">
<label for="rate3">3</label>
<input type="radio" id="rate4" name="rate" value="4">
<label for="rate4">4</label>
<input type="radio" id="rate5" name="rate" value="5">
<label for="rate5">5</label>

    You could then hide the input and style the label according to your need.

    input[type="radio"] {
    display: none;
}
input[type="radio"] + label {
    font-weight: var(--fw-h1);
    width: 40px;
    height: 40px;
    border: none;
    border-radius: 100%;
    color: var(--Light-Grey);
    background-color: hsl(213deg 20% 18%);
    display: flex;
    justify-content: center;
    align-items: center;
    cursor: pointer;
}
input[type="radio"] + label:hover {
    color: var(--White);
    background-color: var(--Orange);
}
input[type="radio"]:checked + label {
    color: var(--White);
    background-color: rgb(126 135 150);
}

    Hope this helps. Happy coding!

  • Amal Karim ✅•1,290
    @amalkarim

    Hi Daniel,

    When I turn on my developer tools in chrome, which is sitting at the bottom of the browser, the <div class="container"> is overlapping with the element after it: <section class="main-content">. I think it's because you set height: 100vh; for the .main-header .container. It's better to remove it, or change it to min-height: 100vh;

    Also, please look at the <div class="main-header-mockup"></div>. In my opinion, it's better to place the image inside it using <img> tag rather than as a background-image. First, it's an important image that has meaning, not just a decorative image. Background image is best suited for an image that is just a decorative one. Please refer to this article about image accessibility. And second, in mobile view, the image is vanished. It can't be seen under the Get Started For Free button. By using <img> tag, that could be prevented from happening.

    That's it from me. Hope this helps. Happy coding!

    Marked as helpful
  • Amal Karim ✅•1,290
    @amalkarim

    Hi Effy,

    At some point, like in the design comparison above, the image height is shorter than the description area. I think it's caused by this style: align-items: center; of the article element. Change it to align-items: stretch; will make sure the image height is always the same as the description area. But, at the same time, it could change the image proportion at certain width. To prevent that, add this style to the image:

    img {
    object-fit: cover;
}

    •••

    Your shopping cart is affected by the style that you actually want to implement to the main image (while in desktop view). It is affected by this style:

    @media (min-width: 600px) {
    img {
        content: url(image-product-desktop.jpg);
        ....
    }
}

    My advice is you better change the selector for the main image by adding class to the main image, so that it won't affect the cart image anymore.

    Hope this helps. Happy coding!

    Marked as helpful
  • Amal Karim ✅•1,290
    @amalkarim

    Hi Eduardo. Great solution!

    I'm just curious as why you chose to use vh unit to define font-size, like for example font-size: 2.6vh;. When I visit your page using mobile view, the text is too small to read. I think it's better to define font-size in html using px, and then define it in other element using rem. For example:

    html {
    font-size: 16px;
}
h1 {
    font-size: 2rem;
}
footer {
    font-size: 0.8rem;
}

    Let me know what you think.

View more comments