Mobile-First responsive 3-column preview card component

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted about 2 years ago

  Hi, Cornflakes!

  You have received amazing feedback from @PhoenixDev22. I would recommend improving your solution by following those suggestions.

  In addition to those, some more suggestions from me.

  • Remove the target="_blank" from all the "Learn More" links. Internal navigation should not open a new tab. It would be annoying for users and also the users have to switch tabs if they want to go back to the previous page. Only use it for external navigation (visiting another website).
  • The border on the .button should exist in the initial state. When on :hover, you only need to change the background-color and the color. Also, no padding is needed on the :hover state.

  I hope this helps!

  Marked as helpful

  2

  ---

  

  Cornflakes•290

  @CornflakesPlusPosted about 2 years ago

  Hey Vanza! Thank you for the incredible feedback. I've made the changes. It was really fun knowing better approaches. 😊

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted about 2 years ago

  @CornflakesPlus You are welcome! 😄 Great job on improving the buttons! 👍

  1
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted about 2 years ago

  Hello Cornflakes,

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

  HTML

  • Use the <main> landmark for the component that wraps the three cards, as using landmarks is important to improve navigation experience on your site for users of assistive technology.

  • About <h1>it is recommended not to have more than one h1 on the page. Multiple <h1>tags make using screen readers more difficult, decreasing your site’s accessibility. In this challenge , as it’s not a whole page, you can have<h1>visually hidden with sr-only. Then swap those <h1> by <h2>.

  • Adding rel="noopener" or rel="noreferrer" totarget="_blank"links. When you link to a page on another site using target=”_blank” attribute , you can expose your site to performance and security issues.

  • 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.

  • Add border-radius andoverflow hiddento the main container that wraps the three cards so you don't have to set border-radius to individual corners.

  • For your question: Add min-height: 100vh to the body that let the body grows taller if the content outgrows the visible page instead and remove themargin: 70px 20px;Also you can add a little padding to the body to prevent the component from hitting.

  Hopefully this feedback helps.

  Marked as helpful

  2

  ---

  

  Cornflakes•290

  @CornflakesPlusPosted about 2 years ago

  I loved everything about this detailed analysis. Thank you so much, Phoenix!!!! I've made the changes and will be mindful of using one <h1> per page.

  0
• 

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  Hi Cornflakes, congratulations on completing the challenge, great job! 😁

  Some little tips for optimizing your code:

  • remove all margin from body
  • after, add min-height: 100vh to body because Flexbox aligns child items to the size of the parent container
  • remove border-radius from sedan and luxury classes and add border-radius: .5rem and overflow: hidden to main tag... it is a much better and less difficult approach to manage
  • add transition on the element with hover effect
  • instead of using px use relative units of measurement like rem -> read here
  • for the small screen like a mobile you can add padding: 1rem; to body

  Hope this help! Happy coding 😉

  Marked as helpful

  1

  ---

  

  Cornflakes•290

  @CornflakesPlusPosted about 2 years ago

  @denielden Hey Deniel. Thank you for taking the time for the feedback. It worked like a charm.😊

  1

  

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  @CornflakesPlus you are welcome and keep it up :)

  1
• 

  Cornflakes•290

  @CornflakesPlusPosted about 2 years ago

  Hmmm.. I've check my solution on VScode offline, and the card is centered perfectly but when I posted this solution, the centering does not match the original design. Can anyone point out the problem?

  1

  ---

  

  Travolgi 🍕•31,400

  @denieldenPosted about 2 years ago

  @CornflakesPlus it could be a loading error ... try to reload the files maybe some changes have not been updated

  1