Skip to content
OverviewSolutions
9
Comments
3
Vas
@VasJM

All comments

  • Felipe Godoy•200
    @FelipeGodoy1Submitted almost 2 years ago

    hey guys 👋 I'm Felipe Godoy and this is my solution to the challenge

    All feedbacks for improvements are welcome.

    ✌️😁

    Stats preview card component

    1
    Vas•110
    @VasJMPosted almost 2 years ago

    Hello, Felipe! 👋 This is a beautiful solution, but one critical error that is affecting the responsiveness of your project is the use of px instead of em. For CSS properties like margin, margin-*, padding, padding-*, width, *-width, height, *-height, and when defining media queries, it is recommended to use relative units like em instead of px which is an absolute unit. I would also suggest not setting a fixed width to your container, and instead use max-width, because changing my default browser font-size to 20px breaks your page, causing the text to overflow the card because you have given it a fixed size. Assuming you know how to convert px to em, your width: 1111px should be max-width: 69.4375em, and if you set a max-width you don't have to set a max-height. Similar px to em conversions need to made to the necessary CSS properties I mentioned above. I hope this makes sense!

    Another little problem, theREADME.md file used in your GitHub repo is the one provided my Frontend Mentor and not your personal one which is the README-template.md file. You have to edit the template file as needed and then rename it (to README.md) after deleting the one provided by FEM!

    I hope I have been of some help, Felipe! Feel free to reach out if you need me to expand on or clarify anything I said, or even if you need assistance with something else 😊

    1
  • Vas•110
    @VasJMPosted almost 2 years ago

    Hiya, Edwin!

    You've done a good job here, but there is definitely room for improvement!

    Before I get into it, I wanted you to know that I was unable to view your code on GitHub. Maybe check to see you've linked it correctly!

    So just going off of your reports, here are some suggestions:

    • Having a main element is very important! Whatever is within your body tag needs to wrapped with the main tag. i.e.:
    <body>
    <main>
        ...
    </main>
</body>

    and not

    <body>
    <div class="main">
        ...
    </div>
</body>

    • Again, every HTML doc needs to have at most one h1 element. In this case, "How did we do?" will be your h1! You cannot use h2-h6 without having an h1 present in your doc first. I see you have used the h2 heading element, which is wrong! You can later adjust the font-size using CSS.

    • Your img element should not have the srcset attribute if it is going to remain empty. Remove it and it should be fine! You only have it in there if there are alternative images.

    Unfortunately I cannot help you with the responsiveness as I do not have access to your CSS code. Though I see you have used vh (60vh) for the height and a percentage value (40%) for the width. My advice would be to use em instead of a percentage value, and you don't have to specify a height as long as you have specified a width! But make sure you set it as max-width and not width.

    Also for font sizes in projects, I suggest you use rem or em instead of px. For example, this project called for a font-size of 15px, so in rem this would be 0.9375rem. You get this value by doing 15px/16px where 16px (or 1rem) is the default font-size on most devices unless you have changed this in your settings. In that case, it would be 15px divided by whatever is your browser's default font-size! So the rule-of-thumb here is to use rem or em over px to make your website more responsive!

    I hope this helps you! Hope you have a good day, Edwin, and good luck 😊

    0
  • Vas•110
    @VasJMPosted almost 2 years ago

    Hi, John! You've done an amazing job here! 🎉

    There are a few issues I would like to address:

    • I noticed is that you have used the section element without having a heading element within it. HTML's section element is only to be used if there is a heading it can be identified with! Since your first section element has an h1 element within it, it does not throw an HTML validation error. Also, take for example, chapter titles in a book. Each chapter is a section with a heading, and this heading is the chapter title, which means you cannot have a chapter (section) without a title (heading)! I hope that makes more sense. Might I suggest using a regular div element instead?

    • Another thing is that you have used a lot of span elements in your HTML, and while this is not an issue, it is hard to know their purpose. What are they for exactly? While I might know the answer to this question, other people who are not familiar with this challenge might get confused. So I would suggest at least adding in a class attribute for specificity, and it would be much easier to select them when adding your CSS styles! Another tip of mine would be to add comments in your HTML as this can also help avoid confusion.

    • Lastly, a README file is very important! I don't know why you chose not to have one, but I would highly recommend you create one for all your projects. When downloading the challenge starter files, FEM gives you a README file template so please use it! A person viewing your code on GitHub would certainly want to know what it is you have created.

    Good luck with your future challenges 😊

    Marked as helpful
    0