Skip to content
OverviewSolutions
6
Comments
23
Dan
@dtp27

All comments

  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Priyanshu!

    That looks really good overall! The one thing I would recommend is to take care of the HTML and Accessibility issues. You want to make sure to include the alt="" property, not just for accessibility, but also incase the image doesn't load for whatever reason.

    Also, adding properties like aria-label is a good idea for anchor tags to describe where the link would take you. Here's a good set of articles from MDN. Hope this helps.

    Happy coding!

    Dan

    0
  • Cristina Kelly•140
    @cristinakellytSubmitted over 2 years ago

    Hi, all! I thought this one would be easy, but it took a while to find a good and proper way to put the hover effect on the image. Anyway, it was a good challenge! All feedback and improvements are welcome =)

    NFT preview card using CSS

    3
    Dan•300
    @dtp27Posted over 2 years ago

    Hi Christina!

    That looks fantastic! I noticed that you're using the BEM naming convention. It was recommended to me that I start using it as well. How have you liked using it and how does it compare with anything you've used in the past?

    Thanks!

    Dan

    1
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi @dhruvjha206!

    Congrats on your first challenge, and welcome to Frontend Mentor! A few recommendations I have on your solution:

    1. I would use another p instead of span so that it's treated like a block component in normal flow instead of an inline component.
    2. It's good that you're using Flexbox to center, but I would add flex-direction: column; to make sure that the card and footer are stacked vertically instead of horizontally.
    3. For the img width, I would use max-width: 100%; so that it responsively adjusts to screen size.
    4. It also seems like you hard-coded the width .container to 1440px, which is the total width of the page the design is at. Instead, I would play around with % for the width of the .container, which will make the card responsively adjust.

    Let me know if you have any questions.

    Happy coding!

    Dan

    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Alexis!

    Pretty good solution overall! A few things I would recommend:

    1. I see you're using Flexbox in the card. I would also use it in the body, since that will then center your entire card on the page. Also include min-height: 100vh; in the body to ensure the content takes up the whole page (i.e. it'll be centered relative to the entire page). 2 . I would also change the width property in your img to max-width: 100%;. This will ensure the image scales down responsively with smaller screen sizes.
    2. I recommend setting the base font-size in the body, then using rem to scale fonts from the different elements up and down. This can make it easier to control all of the fonts.
    3. I don't think you need those <br> elements in your paragraph to match it "line-for-line". Pay attention to the design font sizing and weights, and it should take care of itself.

    Let me know if you have any questions or thoughts.

    Happy coding!

    Dan

    Marked as helpful
    1
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Hammad!

    Pretty good solution overall! Flexbox makes it super easy to center the NFT component on the page. You would to add something like this to your body:

    body {
  min-height: 100vh;
  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
}

    That will center the direct child elements on the page, which in this case is just main (your component).

    Let me know if you have any questions about this, thanks!

    Dan

    Marked as helpful
    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Filip!

    Great solution overall! Make sure you style your horizontal line in the component. If you're using HTML to make a <hr> component, you can do something like this in the css:

    hr {
    border: 1px solid hsl(215, 32%, 27%);
}

    Also, pay attention to the font-weight property for the different elements to make sure they are similar to the design.

    Happy coding!

    Dan

    Marked as helpful
    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Fathy!

    That looks really good! I would make sure your h1 is not too big - one of the best piece of advice I got was to use rems for all of my font sizes when I can instead of px. That made it much easier for me to control all of my font sizes relative to each other.

    Also, what was the reasoning behind splitting out the css into three separate files?

    Happy coding!

    Dan

    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Prasanna!

    Congrats on your first challenge, and welcome to Frontend Mentor!

    Great solution overall. One thing I would recommend to center the overall component on the page vertically is to use Flexbox on the body, along with min-height: 100vh;. The minimum height is to ensure that when flexbox is enabled it is centered relative to the entire view.

    Happy coding!

    Dan

    Marked as helpful
    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Sameer!

    Congrats on your first solution, and welcome to Frontend Mentor!

    Great first solution! A couple things I would recommend:

    • Make sure you pay attention to the colors on the style guide and really analyze if your solution looks like the design. I think overall it looks really good but it seems like the background color is a bit off.
    • Try using min-height: 100vh; in your body CSS. This will have the body take up the entire view from the start and ensure that the background color takes up the entire view as well. It will also ensure that your component is centered on the page.

    Hope this helps. Happy coding!

    Dan

    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi @PGFM!

    Congrats on your first challenge, and welcome to Frontend Mentor!

    Great solution overall! One thing I would pay attention to when comparing the design with the solution is the overall dimensions of the component. Try using percentages instead of pixels to set the width of .inner and play around with getting the solution closer in size to the design. I think all of your text looks great but because the component is larger it looks a little too spaced out.

    Happy coding!

    Dan

    Marked as helpful
    1
  • Yoseif Alfiky•90
    @Yoseif-Alfiky-1Submitted over 2 years ago

    All opinion is acceptable , Tell me what do you think i did it wrong to improve myself. Good luck for every body

    Flex-box and Media query

    2
    Dan•300
    @dtp27Posted over 2 years ago

    Hi Yoseif!

    Great solution! Another way to center your component, because you're actually really close with using Flexbox in the body, is to add min-height: 100vh;. This will make sure the body takes up the entire viewing area, which will result in Flexbox centering the component in the view.

    One thing I would caution about using fixed position is that it takes the element out of normal flow and can cause interesting and unintended side effects when dealing with more components on the page.

    Happy coding!

    Dan

    Marked as helpful
    0
  • Dan•300
    @dtp27Posted over 2 years ago

    Hi Robert!

    Congrats on your first challenge, and welcome the Frontend Mentor!

    Great first solution! One thing I would recommend is setting the min height in the body element to ensure that the background color takes up the entire viewing area and also the component gets centered properly. You can then also use flexbox on the body to center the entire component:

    body {
  min-height: 100vh;
  display: flex;
  justify-content: center;
  align-items: center;
}

    Hope this helps. Happy coding!

    Dan

    Marked as helpful
    0