Skip to content
OverviewSolutions
13
Comments
8
Spirit
@git-ritesh

All comments

  • Spirit•220
    @git-ritesh

    I've seen your solution and I've some suggestions :

    .product_content {
  padding-top: 1rem;
}

.image_avatar {
  height: 24px;
  border: 1px solid #fff;
  border-radius: 50%;
  vertical-align: bottom;
}

.product_img img {
  border-radius: .5rem;
}

.product_content > p:nth-child(5) {
  vertical-align: middle;
  display: inline;
}

.product_price {
  vertical-align: middle;
}

.product_price > img:nth-child(1) {
  vertical-align: middle;
  margin-right: .25rem;
}

.product_original_price > img:nth-child(1) {
  vertical-align: middle;
  margin-right: .25rem;
}

.product_img {
  position: relative;
}

.product_img {
  position: relative;
}
  • Spirit•220
    @git-ritesh

    I've seen your solution the main is not centered. you've tried to center the main & gave display:grid & place-content:center to the body.

    • You should add the min-height: 100vh to the body to make the main centered.
    Marked as helpful
  • Spirit•220
    @git-ritesh

    Following thing should be fixed & improved in your solution :

    • Layout is breaking in Responsive test.
    • The container containing all the cards should have border-radius Rest is Okay. I hope you'll fix it soon :)
    Marked as helpful
  • Spirit•220
    @git-ritesh

    I've seen your solution. it's completely different from the one in the design.

  • Spirit•220
    @git-ritesh

    Your <h1> (HTML & CSS foundations) has no effect on hover please change the color to yellow (given in the style-guide.md.

    h1:hover {
color : hsl(47, 88%, 63%);
}
    Marked as helpful
  • Spirit•220
    @git-ritesh

    I have some simple suggestion you might find interesting. I noticed that you have <h2>Published 21 Dec 2023</h2> in your code When a screen reader is reading the above it will pronounce 21 Dec 2023 as it is. This should be wrapped in <time datetime="2023-12-21">21 Dec 2023</time> This is machine readable therefore it is more accessible for user's with visual impairment and it is also accessible by calender such a google calender. To find out more about the time tag check out this article

    Other than that your project is AWESOME 🤩

    Happy coding !

    Marked as helpful
  • Spirit•220
    @git-ritesh

    I've seen your project. Everything is looking nice except two things :

    1. The image, it's breaking out. Please fix that and it'll be awesome. 😃
    2. align the card to the center it will look nice. ✨
  • Spirit•220
    @git-ritesh

    I often use display: flex;, align-items: center; & justify-content: center; to center things from both sides i.e. horizontally & vertically.

    Though if I have to center a <div> only on the main axis then I use margin: 0 auto; to center but it should have a width for that to be happen as by default any <div> element is display: block; and it would take full width in the parent container.