3 column preview card component

Community feedback

• 

  T

  Grace•31,270

  @grace-snowPosted over 2 years ago

  Hi

  This looks good on my mobile.

  Although I can see some room for improvements in places...

  • it's not good practice to clutter the html with empty divs. If you want to include decorative imagery, either put them in the html or use pseudo elements.
  • those buttons should all be anchor tags not button elements (they would trigger navigation)
  • you're making the styles way harder than they need to be by setting min and max width and setting padding in %. If you put padding in rem it's consistent and controlled. And all these need are max width, no min required
  • similarly, there's no need to set max width on low level elements like paragraphs. Let them bee as wide as they need to - they are already limited by the max width and padding on their container
  • always remember to include focus visible styles (obvious and clear ones, not included in designs) as well as hover styles on all interactive elements

  I hope this is helpful

  Marked as helpful

  1

  ---

  

  mycrochip•460

  @mycrochipPosted over 2 years ago

  @grace-snow Thank you, Grace, I appreciate your feedback.

  0

  

  mycrochip•460

  @mycrochipPosted over 2 years ago

  @grace-snow Thank you once again. This was very helpful.

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted over 2 years ago

  Hi Mycrochip, great work on this challenge! 😉

  Here are a few tips for improve your code:

  • Tip of graphic design: with font-family:" Big Shoulders Display ", cursive the browser will use the Comics Sans font when it doesn't find the first font indicated (you can seen during loading)... for the designer it's a really awful font! I would rather replace it with a font-family:" Big Shoulders Display ", sans-serif much more similar to the primary font.
  • remove all margin from card class because with flex they are superfluous
  • use flexbox to the body to center the card. Read here -> best flex guide
  • after, add min-height: 100vh to body because Flexbox aligns child items to the size of the parent container
  • add transition on the element with hover effect
  • instead of using px or % use relative units of measurement like rem -> read here

  Overall you did well 😁 Hope this help!

  Marked as helpful

  0

  ---

  

  mycrochip•460

  @mycrochipPosted over 2 years ago

  @denielden Thank you for your feedback Deniel. I appreciate it.

  There is so much to take in. I will make sure to be mindful of them in future projects.

  1