Skip to content
OverviewSolutions
8
Comments
46
Akshay
@akshaywebster

All comments

  • Akshay•810
    @akshaywebsterPosted over 2 years ago

    Hey, Kaung!

    Great work on this challenge. Well done!

    0
  • Elyas•300
    @ElyasthrSubmitted almost 3 years ago

    It was a very good experience that allowed me to progress! it is only by practicing that you become efficient.

    Github User Search

    1
    Akshay•810
    @akshaywebsterPosted almost 3 years ago

    Hey, Elyas. Great work on the challenge.

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

    1
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    Hi, Marcin.

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

    0
  • Ted•180
    @sharipoff-0-1Submitted almost 3 years ago

    Any feedback would help me to improve my front-end skills for my next projects.

    Mobile first approach

    3
    Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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! :)

    0
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    1
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    1
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    0
  • Camila Melo•120
    @IllaMeloSubmitted almost 3 years ago

    Hi people!! I'm trying to improve myself as I'm still a noob. If anyone has any suggestions, please, I'm listening!

    nft-preview-card

    2
    Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    0
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    1
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    0
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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
    1
  • Akshay•810
    @akshaywebsterPosted almost 3 years ago

    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! :)

    0