3-column preview card component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello Brotli,

  Congratulation on completing this challenge, I have some suggestions regarding your solution:

  HTML

  • About <h1> it is recommended not to have more than one h1 on the page .You can add a <h1> with class="sr-only" (Hidden visually, but present for assistive tech). Then you can use <h2> instead of those <h1>.

  • In this challenge , all the svg’s are decorative.They do not add significant value). They should be hidden from the screen reader with aria-hidden="true" and focusable:false as some older browsers focus and try to announce them to the screen readers

  • It is essential that interactive elements have focus-visible styles as well as hover styles. These need to be really clear and obvious as they are needed to help a keyboard user know where is focused on the page.

  CSS

  • Changing base html size . This has huge accessibility implications for those of us with different font size or zoom requirements.

  • The component is cut off in my mobile. The media query 375px is too small to change from the desktop to mobile layout .

  • If you make each column into a flex column. and set everything inside the cards to have some margin in one direction marin-bottom: ; only the link wouldn't need it and use margin-top:auto on the learn more link that will push it to the bottom of the cards , then would be (no need for position absolute )

  • In line-height: 25px; use unitless value for the line-height , this is the preferred way to set line-height and avoid unexpected results.Read more in MDN.

  Overall, your solution is good. hopefully this feedback helps.

  Marked as helpful

  1
• 

  Chris•240

  @ChristopherParkePosted over 2 years ago

  It looks really good! the only thing you seem to have missed is the media breakpoint.

  I see you have:

  @media screen and (max-width: 375px) {

  but you should have:

  @media screen and (min-width: 1440px) {

  as per the style guide. Note that I've changed your code from MAX width to MIN width, as is best practice. So you may need to re-arrange some of how you've styled it to make this work. The quick and dirty solution would be just keep max -- but you should get in the habit of using min. The reason is the base styles should be a "mobile first" approach (styling for smallest screen widths), and your media queries should control the style changes once the screen size becomes wider.

  You'll also notice it looks better if you do it this way if you resize the window. Larger mobile and tablet sizes are super squished on your current build.

  Awesome job!

  Marked as helpful

  0