3 Column Card

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, great work on this one. Right now the desktop layout is small and is a bit pushed on the right side. Responsiveness and mobile layout really needs to be looked on this one. But hey, at least you did manage to make something out of right^^

  Agata already gave great feedback on this one, just going to add some suggestions as well:

  • Always have a main element to wrap the main content of the page. On this one , the #container should be using main .
  • Avoid using height property with % or vh unit as this is not consistent for all users. If you try to inspect your layout in dev tools at the bottom, you will notice that the layout is squished since it only gets a portion of the available screen size. Instead, you should use rem value so that it will be consistent.
  • Since you are using position: relative to make the layout centered, what you could do instead is that, first remove all this properties on the #container:

  position
  margin
  top
  height # remove this for now or set a rem value on this 
  width # use Agata's point
  

  Then on the body tag add:

      align-items: center;
      display: flex;
      justify-content: center;
      min-height: 100vh;
      margin: 0;
      flex-direction: column;
  

  This will, the content will be always centered properly. Take note that I used flex-direction: column since there are 2 items directly inside the body tag.

  • Avoid using too much id attribute. When styling always use class to target different elements. id is not used because of css specificity.
  • Each car icons img should be hidden since they are only decorative images so use alt="" and add aria-hidden="true" attribute on every img.
  • Avoid using multiple h1 element on a page, use only at least 1. Change those h1 to h2. But a site needs to have at least 1 h1.On this case, you will make the h1 a screen-reader only text. Meaning it will be hidden for sighted users and only users that uses screen-readers will be able to know about it. Take a look at Grace's solution on this one, inspect the layout and see how she used the h1 and also the stylings applied to it. Copy those since you will need that a lot.
  • Lastly, the mobile state and the site's responsiveness:>

  Aside from those ,great work still on this one. If you have queries, just drop it here^

  Marked as helpful

  0
• 

  Agata Liberska•3,995

  @AgataLiberskaPosted about 3 years ago

  Hi Sebastian! Nice work here, although some things could be improved.

  • I don't think there's a need to set a width for your #container here - just let your content take up as much space as it needs. Also setting a width is not the best choice here - on slightly smaller screens this will put the content off-center. Since you set the display to flex, you can center the content using justify-content: center (margin:auto won't work with flex)

  • It really would be good if you had mobile design here as well, especially that in this challenge, it's quite straightforward. By nature, divs will be in a column anyway, so all you need to do is restrict the width and center it in the body, and then add a media query to set the display to flex as you've got it here.

  • And lastly, my absolute favourite mind-blowing pro tip: you've got your border radius set on the outer cards - you can easily set it on the container if you add overflow: hidden - without it, the corners of the cards will cover those rounded one up :)

  Hope this helps, let me know if you've got any other questions!

  Marked as helpful

  0

  ---

  

  Sebastian•35

  @PulseFictionPosted about 3 years ago

  @AgataLiberska Thanks for the feedback I really appreciate it! I've been using widths in my containers to use margin: 0 auto, thank you for telling me about justify-content, I still need practice with flexbox.

  With the last tip, how do I get my border to fit the content? I still have a border on my container that's the full width of the viewport; I'm assuming something other than width?

  Thank you again for the advice :D

  0

  

  Agata Liberska•3,995

  @AgataLiberskaPosted about 3 years ago

  @PulseFiction Hi Sebastian, sorry for the late reply, I totally missed the notification!

  I was talking about the border-radius. Right now you've got it set on the first and last cards, which means that when layout changes, that property should be updated in a media query to make sure it matches the design. It would be much easier to set that border radius on a container , but to make sure the rounded corners are visible, add overflow:hidden; otherwise it will be covered up. I made a codepen to show what I'm talking about here, I think it will help illustrate my point :D https://codepen.io/agataliberska/pen/vYJZyZd

  0