3-column preview card component

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hi Khan Abdul Rehman,

  Congratulation on completing another frontend mentor challenge. Excellent work! I have some suggestions regarding your solution:

  • You can use the <main> landmark to wrap the body content (which is the three cards) and <footer> for the attribution as using landmarks is important to improve navigation experience on your site for users of assistive technology.

  • Page should contain <h1>. In this challenge ,you may use<h1> visually hidden with class=”sr-only”. You can find it here Then you can use <h2> instead of <h3>.

  • In my opinion, the images are much likely to be decorative. For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images.

  • What would happen when the user click those learn more? In my opinion, clicking those "learn more" would likely trigger navigation not do an action so button elements would not be right. So you should use the <a>. For future use , it's a good habit of specifying the type of the button to avoid any unpredictable bugs.

  • Don't capitalize in html, let css text transform take care of that. Remember screen readers won't be able to Read capitalized text as they will often read them letter by letter thinking they are acronyms.

  • You have used <br> , using <br> is not only bad practice, it is problematic for people who navigate with the aid of screen reading technology. Screen readers may announce the presence of the element. This can be a confusing and frustrating experience for the person using the screen reader. You can read more in MDN.

  • It’s not recommended to use <br> to increase the gap between lines of text or make empty space between elements, use padding / margin styling via CSS. And about using <br> in the <p> to make the paragraph break in new line, You may use max-width to <p> and remove those <br>.

  Hopefully this feedback helps.

  Marked as helpful

  0

  ---

  

  Khan Abdul Rehman•180

  @Khanza2Posted about 2 years ago

  @PhoenixDev22 Wow! I'm just amazed at how much time you took to give such a piece of valuable information, I'll surely consider every point you said and look what I can do to be the best next time, I would be glad if you view my other solutions! Thank you so much!

  0
• 

  Hyron•5,870

  @hyrongennikePosted about 2 years ago

  Hi @Khanza2,

  It looks goods just a few suggestions.

  1. On the main tag replace height with min-height: 100vh to prevent elements from overflowing it's parent container. 2. Remove the height and margin-left: auto on the .container div and set a max-width in pixels e.g max-width: 1170px;

  on mobile change the columns to 100% instead 50%.

  Marked as helpful

  0

  ---

  

  Khan Abdul Rehman•180

  @Khanza2Posted about 2 years ago

  @hyrongennike Thank you hyron, I'll definitely remember what you said!

  0