any feedback is appreciated

Community feedback

• 

  Joseph•770

  @Jos02378Posted about 3 years ago

  Hey @fangyinglim, good job on this solution!

  Some suggestions for you:

  1. Try to Wrap your container inside a main tag to clear some issues.
  2. Try to fix your URL link for the Sedan icon to /images/icon-sedans.svg because it doesn't load on the website.
  3. Try not to set the height of the container explicitly because the cards break in the phone size, try to use min-height: 100vh; or not setting the height at all so the container can expand as the content grows.
  4. I don't recommend you to use em unit for font size because it can cause a problem if there is deep nesting of elements. Here is a link to an article that explains when to use which unit link to article.
  5. You can try to follow a CSS naming convention called BEM. You can watch this video for more information Watch the video.

  I hope this helps, good luck!

  Marked as helpful

  1

  ---

  

  fangyinglim•50

  @fangyinglimPosted about 3 years ago

  @Jos02378 thank you for your feedback! i'll read up on those articles and videos and make these changes :)

  0

  

  fangyinglim•50

  @fangyinglimPosted about 3 years ago

  @Jos02378 hi jos, hope you can clear some of my queries regarding your feedback on my code:

  1. what issues does including a main element fix? is it good practice to include a main element everytime?
  2. so setting a 100vh actually causes the content to break when the page is minimised to mobile? so would creating a media query also fix that in this case?

  0

  

  Joseph•770

  @Jos02378Posted about 3 years ago

  Hey @fangyinglim , these are my answers regarding your question:

  1. so including the main tag will clear up some of the accessibility issues that you see if you check the report above. There is an article explaining why you need that.

  2. Yes for your case, because you specifically set the height of the container to 100vh and the height of the card to 33.3% to the container, some of the content of the cards are cropped in mobile size. Therefore you can't see the lower part of the card since there is not enough height to display it. For your case, you don't need a media query since you approach mobile-first then desktop. You can either use a min-height or not set the height at all for the container in mobile size to fix this problem.

  I hope this helps, good luck!

  Marked as helpful

  0

  

  fangyinglim•50

  @fangyinglimPosted about 3 years ago

  @Jos02378 tysm! you have been really helpful! i appreciate it a lot :)

  0