Responsive 3 column preview card

Community feedback

• 

  Eduardo•910

  @KTrick01Posted over 2 years ago

  Hi! About your main border radius question, the border radius is working, but it looks like it isn't because your car's container corners ( .sedans and .luxury) are overflowing and that's why you can't see the rounded borders, so a quick fix for this is:

  main {
      border-radius: 0.5rem;
      overflow: hidden;
  }
  

  This way you will be able to see the rounded corners of your main container, hope that it helps!

  Marked as helpful

  1

  ---

  

  Shimul•40

  @shimul0022Posted over 2 years ago

  @KTrick01 Thank you so much, it worked!

  0
• 

  Lucas 👾•104,340

  @correlucasPosted over 2 years ago

  👾Hello Shimul, congratulations for your new solution!

  My tip for you is about managing your CSS and semantics:

  You use a single class to control all the cards and one for the characteristic that are different for each, like the color / icon for example. This way you've one single class to control all the cards, like the padding, max-width and etc.

  To improve the semantics you can replace the div wrapping the card with a article.

  👋 I hope this helps you and happy coding!

  Marked as helpful

  0

  ---

  

  Shimul•40

  @shimul0022Posted over 2 years ago

  @correlucas i will definitely go with this class approach from next time. and will also article.

  Thank you so much for your advises.

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted over 2 years ago

  Hello Shimul,

  Congratulation on completing this challenge. Excellent work! I have few suggestions regarding your solution, if you don't mind:

  • Page should have a least one level heading. You can add a <h1> with class="sr-only" (Hidden visually, but present for assistive tech).

  • In this challenge , the images are much likely to be decorative. For any decorative images, each img tag should have empty alt="" as you did 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.

  • Avoid creating duplicate content (duplicate navigation). You can style the same navigation in mobile and desktop differently using media queries. Practice like this can result in a poor user experience, when a visitor finds substantially the same content repeated within a set of search results.

  • There is no need to have the two <p> with the same content, 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.

  • You don’t need to use <br> to limit the maximum width, you can use max-width rule.

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

  0
• Account deleted

  Issue: On mobile version, there's space for paragraph

  Solution: Remove padding from line no. style.css.64

  Or use media queries to fix it for mobile @media(max-width: 450px) { p { padding: 0; } }

  PS: You can connect on Google meet for better understanding. Happy to help :-)

  0

  ---

  

  Shimul•40

  @shimul0022Posted over 2 years ago

  @naincycodes thank you so much. it worked!

  0