Order Summary Component

Community feedback

• 

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 11 months ago

  Hello there 👋. Good job on completing the challenge !

  I have some suggestions about your code that might interest you.

  • Using both min-width and max-width together can be redundant and might not achieve the desired responsiveness, and it essentially sets a fixed width for the component, similar to using width: 403.2px. You can use only max-width without specifying min-width

  • Use min-height: 100vh instead of height. Setting the height to 100vh may result in the component being cut off on smaller screens, such as a mobile phone in landscape orientation.

  • Use an empty alt attribute (alt="") for icons to signal their decorative nature to assistive technologies.

  • Opt for a button instead of an anchor tag for the "change" element, as it involves altering the order's plan. Buttons are more suitable for action-oriented tasks.

  • For desktop devices, use background-size: contain; to make the background adapt to the screen.

  I hope you find it useful! 😄 Above all, the solution you submitted is great!

  Happy coding!

  0

  ---

  

  Chyka•60

  @CrownedTechiePosted 11 months ago

  Thank you so much for your feedback @MelvinAguilar

  • My media query targeted min-width:900px. Initially, I used only max-width, then when I checked it's responsiveness at that 900px screen width, it compressed my container and the content was overflowing. So I had to use min-width to counter that issue. For the max-width, I used it to target the screen widths above 1440px.

  • I checked background-size: contain and I was having some empty spaces, so I opted for background-size: cover.

  0

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 11 months ago

  @CrownedTechie Ah, no, that's fine. I'm referring to the use of min-width and max-width in the .container element. Your adoption of a mobile-first approach with media queries is good. I'm specifically addressing lines 157 and 158 in your CSS:

   .container {
          width: 28%;
          min-width: 403.2px;  /* This line */
          max-width: 403.2px;  /* ... and this line */
  }
  

  Also, be careful with 'cover' because the background tends to occupy (cover) the entire screen, and it might result in distortion

  Happy coding!

  0

  

  Chyka•60

  @CrownedTechiePosted 11 months ago

  Oh alright. Though I'm still confused. So is it better to just use width: 403.2px; to have a fixed width than using both min-width and max-width? @MelvinAguilar

  0

  

  Melvin Aguilar 🧑🏻‍💻•61,140

  @MelvinAguilarPosted 11 months ago

  @CrownedTechie It's better to use max-width: 403.2px; using both max and min at the same time is redundant.

  0

  

  Chyka•60

  @CrownedTechiePosted 11 months ago

  Thank you dear@MelvinAguilar

  1
• 

  Daniel 🛸•44,310

  @danielmrz-devPosted 11 months ago

  Hello @CrownedTechie!

  Your project looks great!

  If you add a box-shadow to the card, it'll look even closer to the original design.

  Other than that, you did an excelent job!

  0

  ---

  

  Chyka•60

  @CrownedTechiePosted 11 months ago

  Thanks dear. I need more practice on box shadowing, I still find it difficult @danielmrz-dev

  1