Skip to content
OverviewSolutions
13
Comments
32
Prantik
@prantiknoor

All 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
  • Johnny•470
    @johnnysedh3llloSubmitted over 2 years ago

    #what-i-found-difficult

    • i found making the site fully responsive on all viewports and orientations a bit difficult to do.
    • i found it hard to efficiently use css units and values
    • i also found it hard to interchange the between desktop image and the mobile image properly.

    #area-of-uncertainty

    • the scalability of my code
    • proper code formatting

    #questions-on-best-practices

    • how can i write better semantic Html?
    • how can i utilize css properties properly especially in regards to the interchanging of the images?
    • what are the best practices in case of accessibility here?

    Responsive Product Preview Web Component Using HTML & CSS Flexbox

    3
    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
  • Johnny•470
    @johnnysedh3llloSubmitted over 2 years ago

    #what-i-found-difficult

    • i found making the site fully responsive on all viewports and orientations a bit difficult to do.
    • i found it hard to efficiently use css units and values
    • i also found it hard to interchange the between desktop image and the mobile image properly.

    #area-of-uncertainty

    • the scalability of my code
    • proper code formatting

    #questions-on-best-practices

    • how can i write better semantic Html?
    • how can i utilize css properties properly especially in regards to the interchanging of the images?
    • what are the best practices in case of accessibility here?

    Responsive Product Preview Web Component Using HTML & CSS Flexbox

    3
    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
  • Petri Saari•120
    @Peteksi95Submitted over 2 years ago

    This is my first project where i used media queries, any feedback what could be done better and more efficiently is very much welcome

    Product Preview Card

    2
    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
  • Sanja•40
    @sdrenjanSubmitted over 2 years ago

    Alignment and spacing inside each card were most difficult to figure out. Is there a better way to do it? Also seems to me like I overcomplicated everything.

    3 column with grid and flex

    1
    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
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hey @nikhil149. I want to give you a recommendation.

    You used height: `${Math.round((value.amount / totalAmount) * 300)}%`;. But it can exceed 100%. So I recommend you to use height: `${(value.amount / maxAmount * 100)}%`;. It will never exceed 100%.

    const amount = data.map((value) => value.amount);
const maxAmount = Math.max(...amount);
// const [totalAmount] = useState(amount.reduce((a, b) => a + b, 0));

              <div
                className={
                  maxAmount === value.amount
                    ? "expenseChart_bottomContainer_chart_bar_max"
                    : "expenseChart_bottomContainer_chart_bar"
                }
                style={{
                  width: "30px",

                  height: `${(value.amount / maxAmount * 100)}%`,

                }}
              ></div>
    Marked as helpful
    1
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hey @nikhil149, You do need to design circle Icons. Because it was given in the starter file.

    Marked as helpful
    0
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hola @Martincillo94. ¡Felicidades! 🎉🎉. Has completado tu primer desafío.

    Quiero darte algunas recomendaciones.

    La tarjeta debe estar centrada. Puedes hacerlo fácilmente usando grid. Como a continuación:

    body {
    background-color: hsl(30deg, 38%, 92%);
    font-family: "Montserrat", sans-serif;
    font-size: 0.8rem;
    color: hsl(228deg, 12%, 48%);

    min-height: 100vh;
    display: grid;
    place-content: center;
}

    Secondly, you have been given two images for both mobile & desktop. You only used one.

    You can use both by using picture.

    <picture class="card__firstSection">
    <source 
       srcset="./imagenes/image-product-desktop.jpg" 
       media="(min-width: 600px)"
     >
    <img src="./imagenes/image-product-mobile.jpg" alt="imgMobile">
</picture>

    En este código, la imagen cambiará de móvil a escritorio cuando el ancho mínimo de la pantalla sea de 600 px o más.

    Marked as helpful
    0
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hey @rorozoamuhammad. I have a recommendation to use object-fit: cover in img. So the image will be looked better.

    main header img {
    width: 100%;
    height: 100%;

    object-fit: cover;
}

    Secondly, You should research about picture tag. It would be better if you used it in this project. 🙂

    Marked as helpful
    0
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hey @justinnvera. Congrats 🎉. You have completed the challenge.

    I have a little recommendation to center the card.

    body {
    display: flex;
    justify-content: center;
    align-items: center;
    background-color: hsl(30, 38%, 92%);
    margin: 0;
    padding: 0;
    border: 0;

    /* Add this line & the card will be on center. */
    min-height: 100vh;
}
    2
  • Prantik•660
    @prantiknoorPosted over 2 years ago

    Hey @Fran505. You can change your code as below to solve many issues.

    .cards {
    /* position: relative;
    top: 18rem; */
    
    display: flex;
    /* You need to change flex-direction row to column in mobile */
    flex-direction: row;
	
    /* You do not need to add this, it automatically be set */
    /* width: 849px; */
    
    margin-left: auto;
    margin-right: auto;

    /* You do not need to add border-radius to other columns */
    /* You can remove border-radius from other columns */
    border-radius: 10px;
    overflow: hidden;
}

.col {
    /* position: absolute; */
    width: 283px;
    padding: 2.5rem;
}

    To center the card:

    main {
    min-height: 100vh;
    display: grid;
    place-content: center;
}
    0