Skip to content
OverviewSolutions
10
Comments
8
Ahmed Hany
@ahmedhanyh

All comments

  • Ahmed Hany•220
    @ahmedhanyh

    Hey @JoseEliasMorales! Great work!

    • Add a css reset to remove some of the default syles applied by the browser (user-agent stylesheet). You can use a small reset like this (by adding it to the very top of the stylesheet):

      * {
  margin: 0;
  padding: 0;
  border: 0;
  box-sizing: border-box;
}

      to remove all margins, paddings, borders, and to include the border size when setting elements' dimensions. Or you can use a larger one like this one. There are more resets out there, you can look them up.

    • Use gap instead of margins/paddings to space the contents inside of each .card element.

    • Change the .card__btn from a <button> to a <a href="#"> because a link is more suitable for this content. When we see 'Learn More` we expect to be redirected to another page, which is how a link behaves.

    • Add cursor: pointer; to .card__btn to change the cursor symbol when hovering over each card button.

    Hope the review was helpful! Wish you the best with your future challenges!

    Marked as helpful
  • Ahmed Hany•220
    @ahmedhanyh

    The point of using heading elements <h1> through <h6> is to give semantic meaning to the text so that screen reader users can understand the structure and importance of that text, and to also make it easier for them to navigate the page. Don't use the heading elements <h1> through <h6> for their default styles applied by the user agent stylesheet, use them to make the site more accessible. Watch this video to learn more and also read this part of this lesson (and take a look at other lessons that focus on semantic HTML from the same course).

  • Ahmed Hany•220
    @ahmedhanyh

    Hey Seymmon!

    margin: auto; or margin: 0 auto; centers the element (or distributes available space) horizontally only and not vertically.

    You can center the container vertically (and horizontally) on the page using flexbox by adding some style declarations to the body, as in this code snippet:

    body {
    ...
    min-height: 100vh;
    display: flex;
    justify-content: center;
    align-items: center;
}

    You can get rid of the margins now.

    It's better to use a layout method like flexbox or grid instead of relying on margins to layout/space the content.

    Resources to learn flexbox:

    I hope the review was helpful! I wish you the best with your future challenges!

    Marked as helpful
  • Ahmed Hany•220
    @ahmedhanyh

    Hey JJ! Amazing work!

    There are some things you can do to improve your solution:

    • You can remove the unnecessary border-style: none; declaration in your css reset as the initial value for the border-style property is already none.
    • The <main> element doesn't need to be a flex container in this case as it contains only a single child element, the display: flex; declaration can be removed. To center the <main> element in the page, give the body a min-height: 100vh and use either flexbox or grid to center it both vertically and horizontally, like this:
    /* Using flexbox */
body  {
  min-height: 100vh;
  display: flex;
  justify-content: center;
  align-items: center;
}

    /* Using grid */
body {
  min-height: 100vh;
  display: grid;
  place-content: center;   /* or place-items: center; */
}

    and remove the margins around the <main> element.

    • Try to use a layout method like flexbox or grid to layout/space the content inside .main_content-wrapper elements instead of using margins.
    • Set the border-radius on the .main_content element with overflow: hidden; instead of specifiying each element corner's radius, like so:
    .main_content {
  border-radius: 0.5rem;
  overflow: hidden;
}
    • To implement the active states for the 'Learn More' buttons, give them a border, change their background-color to transparent and their text color when hovered over using the :hover pseudo-class as in this code snippet:
    a {
  ...
  border: 2px solid var(--color-bg-hd-btn);
}

a:hover {
  background-color: transparent  /* or initial */
  color: var(--color-bg-hd-btn);
}

    That's all I have. I hope you found the review helpful and I wish you the best with your future challenges.

    Marked as helpful
  • Ahmed Hany•220
    @ahmedhanyh

    Hey @mtalha987! Congratulations on completing your project! You've done a great job!

    To answer your question about minimizing the code:

    In HTML

    you don't need to remove anything.

    In CSS

    You can minimize your CSS by doing the following: Move the repeated declarations in the .qrCode, .heading, and .detail rulesets to a new ruleset with a selector that groups them together by separating them with a comma (a grouping selector), like so:

    .qrCode, .heading, .detail {
  margin: 0 auto;
}

.heading, .detail {
  padding-top: 20px;
  text-align: center;
}

    or using the functional pseudo-class selector :is() like so:

    :is(.qrCode, .heading, .detail) {
  margin: 0 auto;
}

:is(.heading, .detail) {
  padding-top: 20px;
  text-align: center;
}

    We can also move the text-align: center; declaration to the body rule (and also remove it from .attribution):

    body {
    ...
    text-align: center;
}

    There are also lots of unnecessary whitespaces across the stylesheet that we can get rid of.

    There are still some improvements that can be made

    In HTML

    Use semantic HTML elements instead of <div>s to improve your website accessibility. Use <main> and <footer> landmarks, and the <h1> heading in your HTML like this:

    <main class="container">
...
  <h1 class="heading">
    ...
  </h1>
</main>

<footer class="attribution">
...
</footer>

    Some useful resources to learn more about semantic HTML and how to use them to structure your content:

    In CSS

    Use max-width instead of width for .container to allow its size to shrink on smaller screen sizes:

    .container {
    ...
    max-width: 300px;
    ...
}

    In general, don't use width and height, use min-width, max-width, min-height, and max-height instead for responsiveness.

    I hope the feedback was useful. I wish you the best with your journey on FEM.

  • Ahmed Hany•220
    @ahmedhanyh

    Hey Nasyr Akhmadov! I hope you're well. Congratulations on completing your project!

    To center the div container both vertically and horizontally, you will need to use one of two layout methods, either using flexbox or grid.

    Resources to learn about flexbox:

    1. MDN's flexbox layout guide
    2. web.dev's flexbox layout module

    Resources to learn about grid:

    1. MDN's grid layout guide
    2. web.dev's grid layout module

    How to use them to center the container:

    1. Using Flexbox: Change the display of the container's parent element (in this case the <body> element) to flex (so that all its children would become flex items on which you could apply flex properties), then you can use the space distribution and alignment properties, justify-content and align-items, to center the container like this:
    body  {
  min-height: 100vh;
  display: flex;
  justify-content: center;
  align-items: center;
}
    1. Using Grid: In a similar manner, you can use CSS Grid to center the div container. Set the display property of the body to grid, then use the space distribution or alignment properties, justify-content (or justify-items) and align-content (or align-items) to center the container like in this CSS:
    body {
  min-height: 100vh;
  display: grid;
  justify-content: center;  /* or justify-items: center; */
  align-content: center;  /* or align-items: center; */
}

    You can also use the shorthand property place-content (or place-items) to set both properties at once, so the above code snippet would become shorter like so:

    body {
  min-height: 100vh;
  display: grid;
  place-content: center;  /* place-items: center; */
}

    In both cases we had to give the body a height, otherwise it would only be as high as its content.

    Hope I've helped. I wish you the best with your FEM journey!

    Marked as helpful
  • Ahmed Hany•220
    @ahmedhanyh

    Hello, @fiqihalfito! I hope you're well. Congrats on completing your project! It looks amazing.

    You've implemented the hover state for the image properly, I have also implemented it similarly. If you want to do it differently, without an empty <div> element, one thing I thought about was using blend modes, using the mix-blend-mode property. For example, you can remove the empty <div> element (the .image-hover) from the HTML markup and add the following styles to your CSS:

    .img-placeholder {
    position: relative;
    cursor: pointer;

    /* Here we've added a background color to the .img-placeholder element */
    background-color: var(--cyan);
}

/* When the .img-fluid is hovered over, it will blend with its background (in the set mode which in this case is 'multiply' */
.img-placeholder:hover .img-fluid {
    mix-blend-mode: multiply;
}

    That will change the color on hover, but it doesn't match the FEM design exactly and I'm not sure if you can accomplish it using this method, but it's an alternate way to get it close to the design without using an extra empty <div> element.

    Also, make the image container's (.img-placeholder) borders round like the image by adding the 'rounded-3' class to it, like so:

    <div class="img-placeholder mb-4 rounded-3">
...
</div>

    Finally, remove the alt text from the view icon (and any icon in your project) and add aria-hidden="true" attribute to it to hide from the accessibility tree and to make the website more accessible. Icons are only decorative and won't have any value for users using assistive technologies. Read more about writing alt text for images.

    I hope I've helped. I wish you the best with your FEM journey!

    Marked as helpful
  • Ahmed Hany•220
    @ahmedhanyh

    Hey! Congrats on completing your project.

    This is an answer to your specific question on aligning the icon with the "Add to cart text"

    One way to achieve this is by using flexbox to align a group of items in a single row or column. Try adding these styles to the element that contains the icon and the text, in this case the div.button_text (which isn't necessary by the way, it's better to remove it, to reduce your HTML markup, and apply the styles below on the <button> instead):

    div.button_text /* or button, or whatever contains the icon and text */ {
    display: flex;
    align-items: center;
    gap: 12px;  /* this only adds a gap between the icon and the text and not necessary for alignment */
}

    Hope I've helped! Wish you the best on your journey.

    Marked as helpful