Skip to content
OverviewSolutions
13
Comments
32

Prantik

@prantiknoor660 points

*I am thinking about what should I write! 🤔*

I’m currently learning...

HTML, CSS & JavaScript.

Latest solutions

View more solutions

Latest comments

  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Oussama Elhousni, Nice work. How did you accomplish the layout of .container > .left is interesting.

    There are some improvements you can do:

    • The links on the dropdown are not clickable. Because that is behind others. By adding a bigger z-index, that works fine.
    • By clicking other than the side nav bar, the nav bar should be closed. You can use the .overlay. You can add an event listener to that to close the nav bar.
    • Instead of switching images by making the display none. There is a better alternative. You can use picture. I believe you can do quick research on that.
    Marked as helpful
    0
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    I have another suggestion. Instead of adding border-radius on both elements, you can add it directly to .product-preview. So then, you don't need to worry about it on mobile/desktop.

    .product-preview {
  border-radius: 10px;
  overflow: hidden;
}

.product-image {
  /* border-radius: 10px 0 0 10px; */
}

.product-main {
  /* border-radius: 0 10px 10px 0; */
}
    Marked as helpful
    1
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Johnny, Congratulations!! 🎉 you have completed your first challenge.

    Your style of asking questions is very well & appreciated.

    I have some feedback.

    .product-preview {
  /* You should give the card width. So you can get more control over your layout. */
  max-width: 40rem;
}

.product-main {
  /* So you don't need to give width here. */
  /* width: 40vh; */
  /* This code will make the main content 50% of the card width. Now the image width will be 50% too. */
  flex: 1 0 50%;
}

.product-image {
  /* You shouldn't hard code this. */
  /* height: 57vh; */
  object-fit: cover;
  height: 100%;
}
    1
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Petri Saari, Congratulations on completing this challenge.🎉

    But you can do some improvement.

    • The main-content size should be smaller.
    • Though by lowering the max-width of main-content, the image won't be small. To lower the size of image, you need to add width: 100% on img.
    • After that, the .text-content will fill more space. to solve this you could add flex: 1 0 50% on .text-content;
    Marked as helpful
    0
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Sanja, Your solution is good and straightforward. Only, you overcomplicated to round the cards. You can easily round the border by only putting the border-radius on the .grid class & adding overflow: hidden;. As your code will be more simple.

    .grid {
    display: grid;
    grid-template-columns: repeat(3, 300px);
    grid-auto-rows: 500px;

    border-radius: 10px;
    overflow: hidden;
}
    Marked as helpful
    1
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Erwin, I want to give you another tip. 💡

    After submitting a solution you can look at others' codes & try to understand how they name classes & ids & compare them with your naming. By doing this you will have a good skill in naming classes & ids. 🙂

    1
View more comments