Skip to content
OverviewSolutions
9
Comments
119
Adeola Ganiu
@Deolabest

All comments

  • Adeola Ganiu•1,320
    @Deolabest

    Hey , Congratulations on completing this challenge!

    Here is my feedback:

    • You're using the span element too much.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    To answer your questions.

    *1. The Add to Cart text and icon are both in the button. Just put them in it like this:

    <button class="btn" type="button"><img src="images/icon-cart.svg" alt="">Add to Cart</button>

    Then you can give the img some margins to create space.

    *2. For the smaller old price, you have to make sure both prices are under one parent elements. i.e.

    <div class="prices">
<strong>$149.99</strong>
<p class="original-price">$169.99</p>
</div>

    Then use this CSS:

    .prices {
display: flex;
margin: 20px;
}

strong {
font-size: 2rem;
margin-right: 15px;
}

.prices p{
font-size: 0.8rem;
margin-top: 12px;
}

    *3. Already answered in the first question.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @0xAbdulKhalid, Congratulations on completing this challenge!

    Here is my feedback:

    • Your design is so amazing. The animation makes it more attractive.

    • The project is not responsive enough for iPad and Tablet users (481px — 768px).

    • Some of the design is cutting out for the mobile users (320px — 480px) too.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    https://www.youtube.com/watch?v=BW0FCFV323s

    Hey @Kuraanal, Congratulations on completing this challenge!

    Here is my feedback:

    • Firstly, you did not upload your project properly on Github. Please watch this video for guidance https://www.youtube.com/watch?v=BW0FCFV323s

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • The project is not responsive enough. The width of the main element should determine where the media query should begin.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Hey @xdcron, Congratulations on completing this challenge!

    Here is my feedback:

    • You don't need a div for main-container when you already have the main element.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @Hermodesign, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    • This property background-color: hsl(212, 45%, 89%); should be under the body element to avoid the white space in the body.

    i.e.

    body {
height: 100vh;
display: flex;
align-items: center;
justify-content: center;
background-color: hsl(212, 45%, 89%);
}

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @Spsingh0005, Congratulations on completing this challenge!

    Here is my feedback:

    1. You're using about 3 different font-family that's making the required one for the project Montserrat not to work.

      • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @ecemgo, Congratulations on completing this challenge!

    Your solutions are top notch and I've learnt a lot from them.

    I noticed the top border-radius is missing from the mobile version.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Hey , Congratulations on completing this challenge!

    Here is my feedback:

    • The thanks page doesn't have the option to rate again.

    • 5 star rating is selected by default which shouldn't be the case.

    NB: I just started learning JS and I'm checking out some related projects here.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Great work!

    You forgot to add the JS file on Github.

  • Adeola Ganiu•1,320
    @Deolabest

    Hey @simplysabir, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @dragonanson, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Hey @YaswanthSaiCh, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    • Also add a little margin to the bottom of the original price.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Hey @nicolas-lukita, Congratulations on completing this challenge!

    Here is my feedback:

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

  • Adeola Ganiu•1,320
    @Deolabest

    Hey @31-wtcr, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @Amadievans, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @Mecha-Mind, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @JoshuaAsistio, Congratulations on completing this challenge!

    Here is my feedback:

    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    • Use <main> instead of a simple <div> to improve the semantics and accessibility on the page. Remember that every page should have a <main> block and that <div> doesn't have any semantic meaning.

    Keep doing a good job!

    Marked as helpful
  • Adeola Ganiu•1,320
    @Deolabest

    Hey @DoyeDesigns, Congratulations on completing this challenge!

    Here is my feedback:

    • To center items at body of a page, just do this:
    body {
    min-height: 100vh;
    align-items: center;
    justify-content: center;
    display: flex;
}
    • It's not a good practice to use px to set font-size. Instead use rem units, they are great since they adapt better to the font-size the user will set in the browser settings. You can convert from px to rem here: https://pixelsconverter.com/px-to-rem.

    Keep doing a good job!

    Marked as helpful