Skip to content
OverviewSolutions
26
Comments
7
Exequiel Arco
@quielLovesLasagna

All comments

  • Exequiel Arco•390
    @quielLovesLasagnaPosted 10 months ago

    Ayos! CHARLES MC VIGO

    Here are some suggestions:

    • Use external CSS instead of internal, this makes your code more cleaner and organized.

    • In the case of your CSS reset:

        *{
      margin: 0;
      padding: 0;
      font-family: 'Inter', sans-serif;
    }

    I suggest you remove the font-family declaration and add it inside the body. The reason for this is that it is more of a good practice to add the properties that affect the texts inside the body because it also gets inherited, and the use case for the universal selector, many devs suggest that you use it for removing default padding, margins, and setting the box-sizing to border-box. You can read more about it here and here

    • I notice an error in this part:
        main img{
      height: 80px;
      width: 8-px; <-------
      border-radius: 50%;
      padding: 25px;
      cursor: pointer;
    }

    • I also noticed an inconsistency in your markup, you added an id for your location and quotes, I suggest (for styling's sake) you use a class and don't use it only for two elements, use it for elements that need it. Using a class for selecting elements to style is a good practice instead of adding ID randomly because you want to style them differently.

    • In this part:

        <div class="links">
      <a href="#">GitHub</a>
      <a href="#">Frontend Mentor</a>
      <a href="#">LinkedIn</a>
      <a href="#">Twitter</a>
      <a href="#">Instagram</a>
    </div>

    This is a list of social media platforms, it's more semantic to use ul and li here. Like this:

        <ul class="links">
      <li><a href="#">GitHub</a></li>
      <li><a href="#">Frontend Mentor</a></li>
      <li><a href="#">LinkedIn</a></li>
      <li><a href="#">Twitter</a></li>
      <li><a href="#">Instagram</a></li>
    </ul>

    That's all! I hope these may help you.

    Marked as helpful
    0
  • Tolulope Oluwabukunmi•240
    @TheTreeveloperSubmitted 10 months ago

    Thanks to Marzuk Sanni for the advice. I was able to start building from the mobile version, then made it responsive for the desktop version. Any feedback is highly appreciated, thank you.

    Responsive Social media profile page

    1
    Exequiel Arco•390
    @quielLovesLasagnaPosted 10 months ago

    Great job, Tolulope Oluwabukunmi! Here are some suggestions:

    • I can see that you did:
     <h1>Jessica Randall</h1>
 <h3>London, United Kingdom</h3>

    Here are some things that you must keep in mind when using headings:

    1. You should only have 1 <h1> element in your document. (main heading/main topic)
    2. You should not have a secondary heading <h2> if you don't have a main heading <h1>
    3. You should not have <h3> if you don't have <h2>
    4. You should not have <h4> if you don't have <h3> and so on...

    Changing your code from the code above to the code below is the correct way of marking it:

     <h1>Jessica Randall</h1>
 <h2>London, United Kingdom</h2>
    • I can also see that you did this:
     <section class="button-container">
      <button>GitHub</button>
      <button>Frontend Mentor</button>
      <button>LinkedIn</button>
      <button>Twitter</button>
      <button>Instagram</button>
  </section>

    this part of the profile is a list of social media platforms. using a ul and li's is the correct way of marking it:

     <ul class="button-container">
      <li>GitHub</li>
      <li>Frontend Mentor</li>
      <li>LinkedIn</li>
      <li>Twitter</li>
      <li>Instagram</li>
  </ul>
    • Inside your external CSS file, I can see that you did not reset your CSS although this is "optional" (?) most/every developer does this so I suggest you do it as well. You can copy my CSS reset if you want to:
    *, *::before, *::after {
margin: 0;
padding: 0;
box-sizing: border-box;
}

img, svg{
mix-width: 100%;
display: block;
}

    You can read more about this here or look it up online.

    That is all. I hope these suggestions may help you.

    Marked as helpful
    0
  • Exequiel Arco•390
    @quielLovesLasagnaPosted 10 months ago

    Nice work, @lekkaaudisy ! Here are some suggestions:

    • I can see that you are doing a great job at the markup but instead of using div tags for each box/card, I suggest you use article tags.

    • Try adding metadata to add additional information about the website or project when you host it on the internet, you can learn more about it here

    • I noticed that you are importing separate fonts inside your style.css, I suggest you select all of the fonts needed for the project and then import them in bulk (which is a common practice) instead of importing them separately.

    That's all :)

    0
  • MateusCamargoS-1•60
    @MateusCamargoS-1Submitted 10 months ago

    Project Challenge Reflection

    This project, no matter how seemingly simple, presented me with a considerable challenge. I'm uncertain about what caused more trouble—whether it was the positioning or decisions related to the image size. Anyway, I would appreciate it if you could take a look and offer any tips you may have. Thank you all very much.

    Solution Stats preview card component - Mobile First and Flexbox

    1
    Exequiel Arco•390
    @quielLovesLasagnaPosted 10 months ago

    Nice work @MateusCamargoS-1 ! Here are some suggestions:

    • Instead of using p tags inside a div to make a list for the "statistics", I suggest using ul tags and li, this is a more semantic way of representing a list of stats.

    • It's nice that you are using a section tag to contain your content but if we think about this, it's better to use the article tag as it is best suited in this case... You can read more about the article tag here
    Marked as helpful
    1
  • Exequiel Arco•390
    @quielLovesLasagnaPosted 11 months ago

    Nice work @muhibkhan2005!

    Your output is looking good but here are some suggestions to improve it:

    • I see that you did:
     <div class="container">
    <div class="container-main">
      <div class="container-img">
        <img class="qrimg" src="./images/image-qr-code.png" alt="qrimg">
      </div>
      <div class="title-heading">
        Improve your front-end skills by building projects
      </div>
      <p class="title-para">
        Scan the QR code to visit Frontend Mentor and take your coding skills to the next level
      </p>
    </div>
  </div>

    Instead of using a <div> to contain your main content, use the <main> element to give it semantic meaning, <div> does not have a semantic meaning which is not accessible. You can read more about the main element here

    • Setting a min-height for the body and not the div with the class=" container it's good that you are using CSS flex-box to center your content but setting the height to the first container is not ideal (this is just a personal preference) but most developers set this property to the body when doing small projects like this, using min-height for the body makes it more responsive especially in cases where your content is bigger than the actual container while height specifies a fixed dimension to an element in general.

    • Use the font and colors required for the project. You can see the instructions inside STYLE-GUIDE.md I think.

    • You are using both internal and external CSS. You did not remove this part and transferred the CSS declaration in your external CSS.

    I hope this will help you improve...

    0
  • nicholasboyce•220
    @nicholasboyceSubmitted 12 months ago

    How exactly did you go about making sure the box shadow was as close as possible? How did you go about making sure your image was sized correctly? Did you put the image dimensions in the HTML img tag?

    CSS Grid, Flexbox, box shadow, box sizing

    1
    Exequiel Arco•390
    @quielLovesLasagnaPosted 12 months ago

    Hi, Nicholas. Nice question! This was the first challenge that I took, to answer your questions:

    How exactly did you go about making sure the box shadow was as close as possible?

    • Using the box-shadow property, we can basically give it 5 values (x-offset, y-offset, blur, spread, and color). I checked your code and it seems you gave a value of -6px to your spread, in my case, this is unnecessary. What I did was:
    box-shadow: 1rem 0 4rem rgba(0, 0, 0, 0.2);

    I only gave it 4 values (x-offset, y-offset, blur, and color) which worked for me. You need to experiment with this to achieve your desired outcome.

    You can check this out to learn more.

    How did you go about making sure your image was sized correctly?

    • I did not define a fixed size for my image, what I did was I added the max-width property to my card container and made the image occupy 100% of the width:
    .container {
  max-width: 30.5rem;
}

.qr-img {
  max-width: 100%;
}

    Did you put the image dimensions in the HTML img tag?

    • No, I prefer adding styles to my elements externally which is the proper way of adding styles.
    0
  • Exequiel Arco•390
    @quielLovesLasagnaSubmitted 12 months ago

    While implementing the hover state of the button, I encountered something that was unexpected: when the button is hovered, it should change its background to a gradient, I was successful in doing that but when I hover away from the button, it flickers, it would be great if someone could explain to me why that is...

    Results summary component

    1
    Exequiel Arco•390
    @quielLovesLasagnaPosted 12 months ago

    Hey guys, I just found out that transition does not work for gradient background that's why my button flickers whenever I hover out. But I just found a quick and easy solution for this. Check out this link if ever you encounter this problem too.

    Kind regards, Quiel

    1