3-column preview card

Community feedback

• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello @Thermonn , I have some suggestions:

  • Body within the body sit:

  • <main>(which is the container of the three cards) instead of the <section>

  • <footer>(which is the FM attribution).

  • You can add a <h1> with class="sr-only"(Hidden visually, but present for assistive tech).

  .sr-only {
  	border: 0 !important;
  	clip: rect(1px, 1px, 1px, 1px) !important; /* 1 */
  	-webkit-clip-path: inset(50%) !important;
  		clip-path: inset(50%) !important;  /* 2 */
  	height: 1px !important;
  	margin: -1px !important;
  	overflow: hidden !important;
  	padding: 0 !important;
  	position: absolute !important;
  	width: 1px !important;
  	white-space: nowrap !important;            /* 3 */
  } 
  

  • You can replace the <h1 > by <h2>.

  • To make the component perfectly in the middle of the page, you can make the body element as a flexbox container and min-height: 100vh;.

  • Instead of setting width in this, consider using max-width. That will let the component grow up to a point and be limited.

  • No need for any heights . Let the content dictate how tall something needs to be.

  • Try to put classes directly on anything you want to style. Then you won't need as many nested css selectors. Never Style on ID'S.(id attribute value must be unique)

  • border-radiusand overflow hidden to the main container(.container) so you don't have to set it to individual corners.

  • For each card

  {
  display:flex;
  flex-direction: column;
  align-items: flex-start;
  }
  

  then set everything inside the cards to have some margin in one direction. only the button would't need. margin-top: auto; for the LINKS Because this is a flex column, margin-top: auto will push it to the bottom of the cards.

  • You should use em and rem units .Both em and rem are flexible, scalable units which are translated by the browser into pixel values, depending on the font size settings in your design.Using px won't allow the user to control the font size based on their needs.

  Hopefully this feedback helps.

  Marked as helpful

  1

  ---

  

  Artem•20

  @ThermonnPosted almost 3 years ago

  @PhoenixDev22 Thank you for the incredibly in-depth and insightful comment!

  • "You can add a <h1> with class="sr-only"(Hidden visually, but present for assistive tech)." I wasn't really thinking about accessibility when building this project, but thank you for the insight!

  • "Try to put classes directly on anything you want to style. Then you won't need as many nested css selectors. Never Style on ID'S.(id attribute value must be unique)" The thing about id's needing to be unique I agree with; that was an oversight on my part. But I thought having the nested selectors makes things easier to understand?? Instead of having a class called '.container__card', I always prefer to do '.container > .card'. Is this bad practice?

  • "border-radius and overflow: hidden to the main container(.container) so you don't have to set it to individual corners." Thank you!! This is a perfect fix

  Overall, thank you so much for taking the time to write this comment.

  0
• 

  Rio Cantre•9,690

  @RioCantrePosted almost 3 years ago

  Hello there! Good job making this project. Looking at your solution, I would suggest the following for you.

  • Instead of using the svg icons as background, use is as image in the HTML structure.
  • Wrap the car-brands in main tag for readability
  • Use footer tag to wrap the attribution content
  • Avoid using duplicated id in one setting with different functions
  • Use a validator to check your solution

  Hope this helps and Keep up the good work!

  Marked as helpful

  1
• 

  P

  Jessica•880

  @perezjprz19Posted almost 3 years ago

  Wait... so I was looking at your page and trying to figure out why your images weren't showing up. I noticed that the Dev tool keeps saying image not found... so I looked at the was actually pushed to the repository. Could it be because you didn't push the folder containing your images to your Github repo?

  0

  ---

  

  Artem•20

  @ThermonnPosted almost 3 years ago

  @perezjprz19 Thank you for commenting, I fixed the issue and updated the live site

  0
• 

  AYUSH RANJAN•170

  @aayusranjanPosted almost 3 years ago

  add img tag before <h2 class="heading">Sedans</h2>

  0

  ---

  

  Artem•20

  @ThermonnPosted almost 3 years ago

  @aayusranjan Thank you for commenting. I fixed the issue with the images not showing up.

  I'm curious why you suggest I use <img> tags instead of what I did? I think using <div> with background-image allows for more customizability.

  0