Skip to content
OverviewSolutions
9
Comments
13
Nelson Nzewi
@nzewi

All comments

  • KELVIN D TEACH•120
    @kelvin-0Submitted about 2 years ago
    1. I just started implementing relative units of measurement(rem) in my code instead of using px, did I use rem correctly by changing the font size of the HTML element for every screen size change?
    2. Should I use grid or flex for this challenge?
    3. Did I write good code using flexbox?
    4. do I have a good readability website?
    5. on the mobile version, how can I achieve the same design as the given design? Thank you I am new to web design and sorry for my bad English. Any help would be appreciated.

    Frontend Mentor - Product preview card component

    1
    Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Nice work completing this challenge Kelvin, you did very well

    Here are my suggestions:

    1.Your implementation of relative units were okay, however, you did not need to change the font size of the HTML element more than once.

    2.Determining which out of flexbox or grid is subjective, any of them will work. Most times, we use grid for layouts and flexbox for smaller components like this.

    3.You wrote good flexbox. You can use the max-width property instead of width, this makes your components more responsive. You can use the gap property to space flex-items instead of using margin too.

    4.Take a look at the report from Frontend Mentor, you have some HTML issues there.

    5.If you make the changes I suggest below, you can achieve the same design.

    6.Don't style your HTML elements with an ID as you did on the #preview-card

    In your main syling, some properties are redundant. The code below works as well.

    main {
    display: flex;
    align-items: center;
    justify-content: center;
    background-color: hsl(30, 38%, 92%);
    min-height: 100vh;
}

    In your preview-card media query, this would work better

    @media only screen and (max-width: 375px)
#preview-card {
    flex-direction: column;
    max-width: 90%;
}

    In the code below, never leave any content hanging, i.e. you should wrap the 'add to cart' in a tag e.g. `<span>Add to Cart</span>

    <button><i class="fa-solid fa-cart-plus"></i> Add to Cart</button>

    Keep solving challenges and asking for feedback and you will improve in no time

    I hope this helps

    Marked as helpful
    1
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing your first challenge Kelvin

    You have a great solution there

    Here are my suggestions for you:

    1.Use relative units like rem and em instead of px for spacing and font sizes, this would make your site more responsive. Learn about them and use them in your next project.

    2.Never forget to add the alt attribute to your image tag. All images must have alternate text to convey their purpose and meaning to screen reader users. It makes your site more accessible.

    3.In your #container styles, you do not need the width property since you have used max-width already.

    4.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your <div> without a class inside a <main> tag. This makes your HTML more semantic.

    I hope this helps

    Marked as helpful
    1
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing your first challenge JOAOVITORWITT

    You have a great solution there

    Here are my suggestions:

    1.Use relative units like rem and em instead of px for spacing and font-sizes, this would make your site more responsive. Learn about them and use them in your next project.

    2.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your container-inner <div> inside a <main> tag. This makes your HTML more semantic and accessible. You can use other semantic tags like button instead of a div for buttons. Also, make sure you have only 1 h1 on your page

    I hope this helps

    Marked as helpful
    0
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing your first challenge Emmanuel

    You have a great solution there

    Here are my suggestions for you:

    1.You don't need media queries to make this challenge responsive. If you follow responsive design principles, it would be enough. I would recommend this [course] (https://courses.kevinpowell.co/view/courses/conquering-responsive-layouts/)

    2.Using relative units like rem and em instead of px would make your site more responsive. Learn about them and use them in your next project.

    3.You can center your card by doing this

    main {
   display: flex;
   justify-content: center;
   align-items: center;
}

    4.I noticed you made your h1 and p flex containers, wrapped your image in a <section> tag and used a <br> tag in your <h1>. Doing these are not necessary as you can achieve your solution without them.

    I hope this helps

    Marked as helpful
    1
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing this challenge Gurv

    You did great on this challenge. Your CSS is very good.

    Here are my suggestions:

    1.Using relative units like rem and em instead of px for your spacing and font-size would make your site more responsive. Learn about them and use them in your next project.

    2.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your card <div> inside a <main> tag. This makes your HTML more semantic.

    3.Pay close attention to the design and put more effort into trying to replicate it. You can make the font-size of your card heading larger.

    I hope this helps

    0
  • Max•40
    @K4zuki-devSubmitted about 2 years ago

    At the start I hat problems of thinking of a way how to even start, then I just started and just tried things out, worked fine, hardest part was centering it so that it would be centered for all devices.

    Centering all of it: transform: translate();

    3
    Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing this challenge Max

    You have a great solution there. Nice trick you used to center the card

    Here are my suggestions:

    1.You can use flexbox as well to center the card

    body {
            display: flex;
            justify-content: center;
            align-items: center;
      }

    2.Use classes to style your elements to make your styles more reusable.

    3.You can use max-width instead of width to make your card more responsive on smaller device widths

    4.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your box div in a <main>.

    I hope this helps

    Marked as helpful
    0
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing this challenge Yashasvee You have a great solution there.

    Your use of flexbox is decent, however, you can improve.

    Here are my suggestions:

    .container {
    width: 300px; 
    height: 460px; 
    background-color: hsl(0, 0%, 100%);
    border-radius: 20px;
    display: flex;
    flex-direction: column;
    justify-content: space-around; 
    align-items: center;
    padding: 15px;
    box-shadow: 0 8px 20px 1px hsl(211deg 16% 74% / 33%);
}

    You have some redundant code there. This code below achieves the same result.

    .container {
    max-width: 300px; 
    background-color: hsl(0, 0%, 100%);
    border-radius: 20px;
    display: flex;
    flex-direction: column;
    padding: 15px;
    box-shadow: 0 8px 20px 1px hsl(211deg 16% 74% / 33%);
}

    1.Use max-width instead of width for more responsiveness. You don't need to specify the height for your card, let the content determine the height. This way it is more responsive

    2.You can use the gap property to space flex-items instead of margins.

    3.Using relative units like rem and em instead of px would make your site more responsive. Learn about them and use them in your next project.

    4.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your container <div> inside a <main> tag. This makes your HTML more semantic

    I hope this helps

    0
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing this challenge RoriCrimson You have a great solution there

    1.Using relative units like rem and em instead of px would make your site more responsive. Learn about them and use them in your next project.

    2.Ensure that all content on your page is contained within a landmark region, e.g Wrapping your card <div> inside a <main> tag. This makes your HTML more semantic

    1. Never forget to add the alt attribute to your image tag. All images must have alternate text to convey their purpose and meaning to screen reader users. It makes your site more accessible

    I hope this helps

    0
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing this project Emmanuel

    You have a great solution there

    1. You can do better by reading through the style.md file well and putting more effort into following the design(fonts, sizes, colors, spacing, etc)

    2.Ensure that all content on your page is contained within a landmark region, e.g <main>

    Marked as helpful
    0
  • Zarko Petrov•210
    @zp021Submitted about 2 years ago

    Lines of text don't match the design photos exactly, as in where the lines break and new lines start. Wasn't quite sure how to achieve this exactly, any feedback regarding this issue or the solution in general is greatly appreciated as this is my first challenge.

    Product preview card component challenge

    2
    Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Congrats on completing your first challenge Zarko

    You have a great solution there

    1.The lines of text don't match the design because you have little padding on the `<div class='text'> Try adding more padding to the left and right sides.

    2.Ensure that all content on your page is contained within a landmark region, e.g <main>

    Marked as helpful
    0
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Great solution Ali Not much to add here but you can do with the <div> wrapping the image as well as the one wrapping the <h1> and <p> elements. This would allow you to write less CSS.

    I hope this helped.

    1
  • Nelson Nzewi•270
    @nzewiPosted about 2 years ago

    Great solution there Rakshith J

    1.<img src="./images/image-qr-code.png" class="img-fluid" alt=""> You can add a value to the alt tag because the image is not merely decorative but conveys something meaningful.

    2.<div class="codeouter"> It would be good if you used more descriptive class names to help other developers understand your code better and also help you in the future to understand.

    3.Use <main> instead of a simple <div> . This way, you improve the semantics and accessibility of your site, showing which is the main block of content on this page. Every page should have a <main> block

    4.The main heading has the tag <h4>. You should replace it with <h1> since this heading is the main title on this page. Every page should have one <h1> to declare the most important title and you should follow the hierarchy using the heading sequence (h1, h2, h3, h4, h5) and never jump a level.

    Marked as helpful
    0