Skip to content
OverviewSolutions
6
Comments
17

Ahmed Mohamed đŸ‡ĩ🇸

@ahmedafsaEgypt đŸ‡ĒđŸ‡Ŧ310 points

I am Ahmed Mohamed, undergoing training to become a Full-Stack Web Developer

I’m currently learning...

JavaScript

Latest solutions

Latest comments

  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello @Singraft, Well done!

    Some suggestions to enhance the code further:

    • CSS Grid is best for the two-dimensional layouts with many elements that need to be precisely positioned relative to each other, while Flexbox is better for one-dimensional or single-line layouts where you just need to space a bunch of elements a certain way. So in case of the card in the challenge, it's better to use CSS Grid like the following:
    .your-container-div {
  display: grid;
  grid-template-columns: 1fr 1fr;
  max-width: 600px;
}

    This will align the product image (left-side) next to the details (right-side) directly, making the code better and easier and It will also help us in making the component responsive by converting the layout from two columns to one column through media query, like this:

    @media(max-width:600px) {
  .your-container-div {
    grid-template-columns: 1fr;
  }
}
    • Regarding the product image It is better to use the <picture> element with <source> tags to make the product image responsive where based on your screen width, the appropriate image will be loaded
    <picture>
  <source
    srcset="images/image-product-mobile.jpg"
    media="(max-width: 600px)"
  />
  <source
    srcset="images/image-product-desktop.jpg"
    media="(min-width: 601px)"
  />
  <img src="images/image-product-desktop.jpg"/>
</picture>

    I hope these suggestions are helpful. Best wishes to you!

    Marked as helpful
  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello abiskar100, Well done!

    • For the text organization and white spaces inside the product details side you can group them all in a div and apply padding to leave some space around the edges. Then use flexbox and gap property to control the spacing between them, like the follwing:
    .product-info {
  padding : 3rem
  display: flex;
  flex-direction: column;
  gap: 2rem;
}

    Don't forget to adjust the numbers because I set them randomly as examples :P

    Some suggestions to enhance the code further:

    • CSS Grid is best for the two-dimensional layouts with many elements that need to be precisely positioned relative to each other, while Flexbox is better for one-dimensional or single-line layouts where you just need to space a bunch of elements a certain way. So in case of the card in the challenge, it's better to use CSS Grid like the following:
    .your-container-div {
  display: grid;
  grid-template-columns: 1fr 1fr;
  max-width: 600px;
}

    This will align the product image (left-side) next to the details (right-side) directly, making the code better and easier and It will also help us in making the component responsive by converting the layout from two columns to one column through media query, like this:

    @media(max-width:600px) {
  .your-container-div {
    grid-template-columns: 1fr;
  }
}
    • Regarding the product image It is better to use the <picture> element with <source> tags to make the product image responsive where based on your screen width, the appropriate image will be loaded
    <picture>
  <source
    srcset="images/image-product-mobile.jpg"
    media="(max-width: 600px)"
  />
  <source
    srcset="images/image-product-desktop.jpg"
    media="(min-width: 601px)"
  />
  <img src="images/image-product-desktop.jpg"/>
</picture>
    • You can add a nice hover effect to the button and use cursor: pointer to inform the user that this button is clickable
    .button:hover {
  background-color: hsl(212, 21%, 14%);
  cursor: pointer;
}
    • The structure could benefit also from more semantic HTML elements like <header>, <main>, <section>, or <article>. They help screen

    I hope these suggestions are helpful. Best wishes to you!

    Marked as helpful
  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello @0xmaxx1, Well done!

    Small suggestions to enhance the code further:

    • Regarding the product image It is better to use the <picture> element with <source> tags to make it responsive where based on your screen width, the appropriate image will be loaded instead of the two <img> elements.
    <picture>
  <source
    srcset="images/image-product-mobile.jpg"
    media="(max-width: 600px)"
  />
  <source
    srcset="images/image-product-desktop.jpg"
    media="(min-width: 601px)"
  />
  <img src="images/image-product-desktop.jpg"/>
</picture>
    • You can use cursor: pointer on the buttons and transition: all 0.3s on the elements with hover effect to apply nice hover effects

    I hope these suggestions are helpful. Best wishes to you!

  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello @nemoanderson, well done!

    Small suggestions to enhance the code further:

    • You're using vh unit for the card height which isn't the best practice; as due to the page's height the card's height gets defined and this can distort its design we want. so consider using pixels (px) or rems (rem). Alternatively it might be better not to set a specific height and let the content determine the appropriate height.

    • The image overlay in the hover effect extends beyond the image's dimensions. You can fix this by using either of:

    .image-container {
    border-radius: 25px; /* Adjust based on the image border radius */
    overflow: hidden;
}

    OR

    .image-overlay {
    border-radius: 25px; /* Adjust based on the image border radius */
}
    • Bonus note: Use transition: all 0.3s on the elements with the hover effect to apply a wonderful transition effect

    I hope these suggestions are helpful. Best wishes to you!

  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello @njabulo-t, good job! just quick notes to enhance the code

    • I noticed that you've centered the card vertically using margin, which not consistently maintain the card at the center particularly when the screen size changes ,Here's a simple trick to ensure the element remains centered:
    .your-container-div /* or body */ {
  min-height: 100vh;
  display: flex;
  align-items: center;
  justify-content: center;
}
    • Also consider enhancing accessibility by integrating Semantic Elements such as <main>, <section>, or <article> into your code.
  • Ahmed Mohamed đŸ‡ĩ🇸â€ĸ310
    @ahmedafsa

    Hello @ilyesmkb, good job!

    just a quick note I noticed that you've centered the card vertically using margin, which might not consistently maintain the card at the center particularly when the screen size changes

    Here's a simple trick to ensure the element remains centered:

    .your-container-div /* or body */ {
  min-height: 100vh;
  display: flex;
  align-items: center;
  justify-content: center;
}
View more comments