Stats preview card with grid, flexbox and SASS

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  Hi

  1. You should not be using max-width in %. You lose all control of the width that way. Instead, set max-width in rem. Eg 80rem for desktop, 30rem for mobile
  2. Media query values should always be written in either rem or em (not min-width 1024px). That is also pretty late to make the switch to the large screen view. I think there is room for the large screen layout before 1000px wide
  3. Instead of repeating img tags and changing their display property, just use the picture element and let the browser change the images for you! Much better for performance and less css
  4. The text area of the card should not be a section element. This is all one chunk of related content. You could make the whole card a section inside main if you wanted (but not necessary) but should not have section elements within the card
  5. Put padding on the text area of the card. You do not need any more nested divs within that half of the card
  6. Use a paragraph tag in this. Small is for smallprint and should only be used inside a paragraph or other element. It is not a replacement for a paragraph. This is not smallprint anyway, it is just normal text content.
  7. The stats should not be in a div with paragraphs. This is clearly meant to be a list of 3 stats, so use an unordered list element. Remember if you change a list to display flex and remove list-style it is necessary to restore list semantics with role="list" on the ul and role="listitem" on each li
  8. The words do not need wrapping in an emphasis tag (em). Use a span, which is meaningless, as the only reason you're wrapping them in anything is for layout purposes
  9. You are misusing padding in this. You should be using margin to create space between elements, like vertical margin between text elements in the card. Padding is for internal spacing and does not collapse; Marging is for external spacing and does collapse (unless used in flex-grid children, then it does not collapse)
  10. You should not be nesting CSS selectors like you are doing atm. You want to keep CSS specificity as low/flat as possible. That means the ideal is single class selectors at all times.
  11. The style guide tells you the body font size in px, but you must always convert values like that to rem before using in your stylesheet. Font-size, line-height and letter-spacing must never ever ever be in px. Very important for accessibility so users font settings can be honoured
  12. The paragraph text in this card should be the size defined in the style guide (it can be inherited from the body). Yours is smaller than that atm.

  By the way, the Lexend Deca 400 font is used for the stats words

  Marked as helpful

  2
• 

  Grace•140

  @gws-jenny-andrewsPosted almost 2 years ago

  Question 1:

  You can remove the inline-block from the image wrapper element and then set the img to have:

  height:100%; width:100%; object-fit: cover;

  or you could switch display:inline-block to: display:flex to the image-container and then make the img:

  flex-grow: 1; object-fit: cover;

  and remove the max-width:100%

  This will resolve the sizing issue of the image. essentially in this case, I believe you want your image to always be the full width and height of the containing div and to use "cover" to "resize" the image.

  Question 2: After a quick look, you should apply "display: flex" to the .stats class and not the p tag in the media query.

  you can then add

  gap: <xx> px / rem etc to the .stats class to spread them out.

  You'll also need to override some of the other classes in the media query to left align the other elements and to add some extra padding.

  Hope this helps.

  Marked as helpful

  1

  ---

  

  Mateo Rinland•210

  @mateorinlandPosted almost 2 years ago

  @gws-jenny-andrews That's extremely helpful. Thanks a lot!

  1