Order summary component using CSS Grid

Community feedback

• 

  Muhammad Shajjar•1,100

  @muhammadshajjarPosted over 3 years ago

  Well done, I would like to add some points,

  As @grace-snow mentioned in her comments that,

  • Proceed and cancel both imply navigation so should probably be anchor tags, not buttons

  • I think you forgot to add active states.

  Marked as helpful

  1

  ---

  

  Rachana Hegde•155

  @rachanahegdePosted over 3 years ago

  @muhammadshajjar I have added active states to the links and buttons but I will make sure to change the buttons to anchor tags instead. Thank you for the feedback! :)

  0
• 

  Yashasvi Singh•890

  @aUnicornDevPosted over 3 years ago

  The font-family for the links(change, proceed payment and cancel) can be set to "Red Hat Display" to match the design.

  Not an overuse of ''DIVS" but some extra "buttons" that could have been just replaced with divs(plan-btn) or a tags(cancel-btn). Semantically speaking, buttons should be signifiers that something can be clicked.

  Marked as helpful

  1

  ---

  

  Rachana Hegde•155

  @rachanahegdePosted over 3 years ago

  @aUnicornDev Ohh okay, I forgot to style the links. I'll make sure to update the code.

  I'll also replace the buttons with divs or a tags, I had assumed that the design required me to code buttons specifically. Thank you for the feedback!

  0
• 

  P

  Patrick•14,265

  @palgrammingPosted over 3 years ago

  I would have just attached your main background to the body tag. But while you currently have a width on your main background image you should add a height: 40vh similar to this

  1

  ---

  

  Rachana Hegde•155

  @rachanahegdePosted over 3 years ago

  @palgramming Thanks for the feedback, Patrick. I'm a little confused though, why do I need to add a height to the main background image? I thought it was already being displayed correctly without the height explicitly stated.

  0