Skip to content
OverviewSolutions
8
Comments
46
Akshay
@akshaywebster

All comments

  • Akshay•810
    @akshaywebster

    Hey, Kaung!

    Great work on this challenge. Well done!

  • Akshay•810
    @akshaywebster

    Hey, Elyas. Great work on the challenge.

    Just a small typo: Joinded in the date field. Please fix it.

  • Akshay•810
    @akshaywebster

    Hi, Marcin.

    For some reason, your live preview site is not accessible. Please look into it.

  • Akshay•810
    @akshaywebster

    Hi, Ted.

    Great work on the project.

    A couple of things to notice:

    • There's no background color for the body that takes the lower half of the screen other than the curved SVG image, so you can apply the following style to fix the issue:
    body {
  background-color: hsl(225, 100%, 94%);
}

    Other than that, it's all good. Keep up the good work.

    I hope my feedback is helpful to you. Have a great day! :)

  • Akshay•810
    @akshaywebster

    Hi, there!

    Great work with the project!

    A couple of things to notice:

    • You've set your container's width to be 935px on screens greater than 700px with the help of the following media query:
    @media (min-width: 700px) {
    main {
        flex-direction: row-reverse;
        text-align: left;
        width: 935px;
        height: 370px;
        margin: 150px auto;
    }
}

    So, it adds a horizontal scroll bar for screen sizes from 934px to 700px width.

    Here's a screenshot.

    What you can do is assign the main container a max-width of 900px, that way it won't have to force it on smaller screens.

    • Also, I noticed your container is not vertically centered, so please look into that. It should be fairly simple since you're already using Flexbox.

    I hope my feedback is helpful to you. Have a great day! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Nitin!

    I think you've done a great job with the project as far as responsiveness is concerned.

    I noticed this line of CSS in your code:

    @media screen and (max-width: 50.9375em) {
  .container {
    width: 17.4375rem;
  }
}

    Something I prefer doing for the mobile view is to put a relative unit for the container's width like:

    .container {
  width: 90%;
  max-width: 450px; /* or something else, it doesn't have to be just 450 */
}

    What this does is it makes your container a relative width to the mobile's screen size, and then it stops the container's width from expanding at 450px after a certain screen size.

    If there's any specific question about responsiveness that you want to ask, feel free.

    I hope my feedback is helpful to you. Have a great day!

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Florin.

    Nice work with this project. Keep it up, mate!

    To get rid of the Accessibility issues:

    • Make sure you have one h1 tag in your HTML document.

    • Your HTML document should have one main landmark. You can achieve this either by using semantic HTML tags or by declaring your main div with role="main".

    It's always a great idea to learn about HTML5 semantic tags to reduce errors like this as well as to make your site more readable by screen readers.

    Here's a great video on that: HTML5 Semantics

    • Also, your card is not vertically centered, so please look into that if you have some free time.

    Overall, you've done a great job since this is your first project here on FrontEnd Mentor.

    Hope my feedback is helpful to you. Have a great day, Florin! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Camila.

    Your project is looking great. Keep up the good work.

    One thing I would advise you to keep an eye on from your next project is to learn about Relative units in CSS. You're using absolute units such as px a lot. It would help you a lot if you start using relative units like em, rem, etc.

    Here's a cool video: Learn CSS Units In 8 Minutes

    Hope my feedback is helpful to you. Have a great day, Camila! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Naveen!

    Great work with the project.

    To get rid of the Accessibility issues:

    • Make sure you have one h1 tag in your HTML document.

    • Your HTML document should have one main landmark. You can achieve this either by using semantic HTML tags or by declaring your main div with role="main".

    It's always a great idea to learn about HTML5 semantic tags to reduce errors like this as well as to make your site more readable by screen readers.

    Here's a great video on that: HTML5 Semantics

    • Coming to your CSS, you're using a lot of px values, it's always great to use relative units such as em, rem and viewport units such as vh, vw.

    Here's an article by MDN: CSS Values & Units

    I hope my feedback is helpful to you. Have a great day, Naveen! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Wow! You did an amazing job with this, Christian.

    It's pixel-perfect!

    Some share functionality would have been nice, though.

    Have a great day!

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Saeed.

    Nice work with the project.

    A couple of things I noticed:

    • You're using a lot of media queries, and while that's a great thing, you can make your CSS more concise by grouping multiple styles for the same screen size in just one media query.

    For example, you've used two media queries in your code like so:

    @media only screen and (max-width: 375px) {
  html {
    font-size: 50%;
  }
}

@media only screen and (max-width: 375px) {
  .container {
    width: 90%;
  }
}

    You can just remove the redundancy by putting the style for both html & .container in just one media query:

    @media only screen and (max-width: 375px) {
  html {
    font-size: 50%;
  }
  .container {
    width: 90%;
  }
}
    • Sometimes in your HTML, you're using certain elements with class names like <h1 class="h1">Order Summary</h1> and <p class="p">. You'd be better off without using classes if you don't want any special styles for some of your elements, or you can make your class names a bit descriptive for better code readability.

    I hope my feedback is helpful to you. Have a great day! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Great work with the project!

    Although I would suggest you to use relative units like em & rem instead of absolute units like px.

    Check out this video for more info.

    Have a great day! :)

  • Akshay•810
    @akshaywebster

    Hi, Dan!

    I took a quick look at your CSS and seems like you're using multiple media queries for a single screen size, i.e. 600px.

    So, to answer your question, no, there's no need to make multiple media queries for the same screen size. You can just put all your classes into one media query.

    For example:

    @media only screen and (max-width: 600px) {
  .ContentContainer {
    margin-top: 10%;
    width: 350px;
    height: 60rem;
  }

  img {
    position: absolute;
    width: 350px;
    height: 400px;
    border-radius: 0;
    border-top-right-radius: 10px;
    border-top-left-radius: 10px;
  }

  .box {
    border-radius: 0;
    border-top-right-radius: 10px;
    border-top-left-radius: 10px;
    width: 350px;
    height: 400px;
  }

  .TitleHeading {
    padding-top: 115%;
    padding-left: 0;
    text-align: center;
  }

  .statsContainer {
    display: grid;
    padding-left: 0;
    width: 350px;
  }

  dd {
    text-align: center;
  }

  dt {
    text-align: center;
  }
}

    I also had this confusion early on when I was starting learning CSS, but I took this course on Udemy and it really me clear a lot of confusion. Here's the link. PS. I am not affiliated to the course instructor in any way.

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, there!

    Great work on this challenge!

    However, your card is not vertically centered, so either try to center with absolute positioning or with Flexbox.

    Also, the horizontal divider is missing above the stats section.

    You need to implement a box-shadow on the bottom of the card as well.

    Other than that, it looks really nice! Keep up the good work.

    Hope my feedback was helpful to you. If you need any help, please feel free to ask. Have a great day!

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Great work on the challenge!

    For some reasons, your background image is not visible. Please look into it, if you have some free time.

    Other than that, it's looking pretty good.

  • Akshay•810
    @akshaywebster

    Hi, Akash.

    Here's a nice tutorial on Mobile First Web Design which you may find helpful: Link

  • Akshay•810
    @akshaywebster

    Hi, Massoud!

    • I think using background-size: contain on the background svg would solve your problem, then there won't be a need to scale anything.

    • Also there was no need to use the two rectify divs at all.

    • To make the hero svg span the entire width of the card, you can just use width: 100% on the img.

    • There was no need to use relative position on the box div, as Flexbox could have taken care of the position of the elements inside it for you.

    I hope my feedback is helpful to you. If you need any further help, feel free to ask. Have a great day! :)

    Marked as helpful
  • Akshay•810
    @akshaywebster

    Hi, Max

    Congrats on your first submission.

    A couple of things I noticed:

    • You're using internal CSS in the HTML file itself. It's something that's not recommended. Not only it makes the CSS hard to maintain, but I felt uncomfortable reading & understanding your code quite difficult because every time I had to scroll up & down to see both CSS & HTML. So, please start using external CSS from your next project.

    • I noticed something weird in your annual-box div. You have created two anchor tags for the Change button, and you're using hidden effect on one button. I don't understand what was the need to do that.

    • If you wanted to place the button to the very right, you could have put both the icon svg & pricing text into one div and the Change button in another element. Kinda like:

      <div class="plan-box"> 
        <div class="plan-left">

    <img src="./images/icon-music.svg" alt="music" />

    <div class="plan-text">

      <h2>Annual Plan</h2>

      <p>$59.99/year</p>

    </div>

  </div>

  <a href="#">Change</a>

</div>

    And do justify-content: space-between on the .plan-box class.

    • To answer your question about setting heights & widths in px, if you want your container or card to move according to the screen size in terms of width & height, it would be better to use relative units like %. Also, you should look into the concept of max & max width & height properties.

    I hope my feedback is helpful to you. Have a great day!

    Marked as helpful