Skip to content
OverviewSolutions
6
Comments
17
Ahmed Mohamed 🇵🇸
@ahmedafsa

All 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;
}
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @Owner-test!

    I noticed that you're using a percentage value for the component's width. this approach could distort the appearance of the component, especially when the screen size changes

    It's better to use rems or pixels such as max-width: 18rem or max-width: 300px for example!

    Best wishes to you!

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @HosseinHeydarpour, well done!

    I'd like to offer a couple of suggestions:

    • I noticed there's a stray end style tag in your HTML code that might need attention.
    • Consider enhancing accessibility by integrating Semantic Elements such as <main>, <section>, or <article> into your code.

    Besides these minor points, your work looks excellent!

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @kylecloete, great work!

    • 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>

    Best wishes to you!

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @Muhammadshipon, great work!

    Just small note it's better to use the font in the style-guide.md file to make the solution more closer to what's required.

    For this QR code component: the font, its weights and size are:

    • Family: Outfit
    • Weights: 400, 700
    • Font size (paragraph): 15px

    You can do this by opening the font page on Google Fonts and linking it in your HTML head

    • HTML:
    <head>
  <link rel="preconnect" href="https://fonts.googleapis.com">
  <link rel="preconnect" href="https://fonts.gstatic.com" crossorigin>
  <link href="https://fonts.googleapis.com/css2?family=Outfit:wght@400;700&display=swap" rel="stylesheet">
</head>
    • CSS:
    body {
  font-family: "Outfit", sans-serif;
}
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @khalid225, good job in your first challange. keep going!

    The fonts and colors are always available in the style-guide.md file within the starter files you can download from the Challange hub page

    For this QR code component: the font, its weights and size are:

    • Family: Outfit
    • Weights: 400, 700
    • Font size (paragraph): 15px

    You should link it from Google Fonts and then use it as font-family: "Outfit";

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @Asumpta-1, great work!

    Here are some notes for improving the code:

    • 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 next to the details 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 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>

    Best wishes to you!

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @Taannn, Good job!

    It is better to use the <picture> element with <source> tags and media queries to make the product image responsive instead of the two <img> elements

    • HTML:
    <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>
    • CSS:
    @media (max-width: 600px) {
  .your-grid-container {
    grid-template-columns: 1fr;
  }
}
    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @Saitenhexer, Good job!

    You can employ the <picture> element with <source> tags and media queries to make the product image responsive

    • HTML:
    <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>
    • CSS:
    @media (max-width: 600px) {
  .your-grid-container {
    grid-template-columns: 1fr;
  }
}
    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello @nemoanderson, Good job!

    To create a layout for displaying both the current and old price place them within a div and assign it a class for example "price-div". Then utilize the display: flex declaration to make both the elements on the same line and use gap to control the space between them

    .price-div {
  display: flex;
  gap: 15 px;
}
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hello, great work!

    Here's my solution to implement the hover effect. I hope it proves helpful 3>

    .equilibrium {
  position: relative;
  overflow: hidden;
  border-radius: 16px;
}

.overlay {
  background: hsla(178, 100%, 50%, 0.4);
  position: absolute;
  top: 0;
  left: 0;
  height: 100%;
  width: 100%;
  display: flex;
  align-items: center;
  justify-content: center;
  opacity: 0;
  transition: 0.5s ease;
  border-radius: 16px;
}

.overlay:hover {
  opacity: 1;
}

    The transparency in the overlay div background comes from adjusting the Alpha in the color code to 0.4 (40%), ensuring it doesn't impact the view icon opacity.

    Marked as helpful
  • Ahmed Mohamed 🇵🇸•310
    @ahmedafsa

    Hey Susmitha,

    I've reviewed your code, and it's impressive! Here are some suggestions to further enhance it:

    • The design currently exhibits excessive white spaces due to the using of .details { justify-content: space-between;} It might be beneficial to remove this and opt for {gap:} instead.
    • Consider increasing the font size of the current and old prices to better catch the user's eye.
    • For the button:
    1. use margin instead of padding to manage the space between the text and the cart Icon.
    2. It's preferable to encompass the entire button with display: flex to align the text and image precisely in the center, using gap to control the space between the cart icon and the text and using padding to control the button's height and width
    .order-button {
  display: flex;
  align-items: center;
  justify-content: center;
  gap: 16px;
  padding: 20px 64px;}
    • I observed that you centered the card vertically using padding: 30px which might not maintain the card at the center consistently especially when the screen area changes Here's a simple trick to ensure the element stays centered:
    .container /* or body */ {
  min-height: 100vh;
  display: flex;
  align-items: center;
  justify-content: center;
}
    • The code appears to lack responsiveness on mobile phones. To rectify this, you can enhance it by employing the <picture> and <source> elements instead of <img> to display the product image. and use the following media query to ensure responsiveness on smaller screens:
    <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>

    @media (max-width: 600px) {
  .your-grid-container {
    grid-template-columns: 1fr;
  }
}

    Apart from those minor points, your code is excellent! Best of luck for you.

    Marked as helpful