3 column card component

Community feedback

• 

  Chris Soncrant•60

  @chrissoncrantPosted about 2 years ago

  Nicely done Mary!

  The responsiveness is good! Based on how you used the media query I see you built this using a mobile-first approach, which is awesome.

  I like that you included a style-guide. That's a nice touch.

  Some things I noticed: On the body the font-size is specified using pixels, which should be avoided because this would overwrite browser preferences set by the user. I am curious why you chose 15px. Was this specified somewhere? If you removed this font-size declaration altogether it would default to 16px or whatever the user sets in their browser preferences and you can still use the rems. Other than this I liked the usage of relative units throughout. Also, here's an awesome article on units that has helped me a lot: https://www.joshwcomeau.com/css/surprising-truth-about-pixels-and-accessibility/

  If you turn the container div into a main element and the column divs into section elements (each with a class of section-1, section-2, etc) that would take care of 2 of the accessibility issues and make the markup semantically happier. Plus using the class name "column" is only accurate when the media query is active, otherwise they are rows. Naming ain't easy though.

  As for the other accessibility issue, I don't see that an h1 would be applicable with this site... I think changing the first h2 into an h1 and styling it the same as the h2s would be perfectly fine though. With that said I would have used h2s just like you did.

  Overall the code formatting looks good and legible, though there are some spacing inconsistencies in the css, but it's really no big deal.

  Again, good work!!

  0

  ---

  

  Mary•30

  @mgracnazarenoPosted about 2 years ago

  @chrissoncrant thank you for taking a time to write me a feedback. I am definitely taking notes of your tips. I will implement them to my next projects. I really appreciate your recommendations. Take care. =)

  0