3-Column Preview Card Component with Flexbox and Grid

Community feedback

• 

  Emma•320

  @emjogalePosted over 2 years ago

  Hi Adrianna, well done on your solution - it looks good. However in response to your question about the media query - I think that it may have been easier if you had used the mobile first approach - then add in the media query for wider screens. With your present media query the layout only changes at 375px or less - but some phones are wider than this - if you check how your layout looks on a screen say 414px wide, the cards are very tall and narrow - it would benefit from being in the 1column grid layout probably from around screen widths of 900px and below. With the wider screen layout, apart from changing the grid you don't have to change much apart from adding a maximum width - probably in rems rather than percentage - as this gives you better control over the size. I have a few other feedback points: The icons didn't show up when I looked at your site - I think the path to them just needs updating with a . in front (./images...). I also suggest you use <a> tags rather than <button> for 'learn more' - as presumably it would take you to another page. One final thing, which is cosmetic - if you have a border on the 'learn more' 'buttons' which is originally either transparent or same colour as the background - then the design won't shift when it is hovered over as it is already in place. I hope these points are of help!

  Marked as helpful

  0

  ---

  

  Adrianna Thomas•70

  @adrianna-thomasPosted over 2 years ago

  @emjogale Thanks so much for your feedback and suggestions. I made edits to the html and css. I hadn't thought about doing the mobile first approach. I tried it out but am having trouble with it being centered on the page. I did "margin: 0 auto" but it's staying at the top of the page.

  0

  

  Emma•320

  @emjogalePosted over 2 years ago

  @adrianna-thomas - well done on the edits! To sort out the centering issue just change the height property on the body to min-height: 100vh; . The width isn't required on the body element. One other point - the accessibility issue you are having with the h1 - is occuring because of the display being set to none - this means a screen reader won't read it. It is best to set some specific screen reader only styles for this type of heading. This site explains it well: webaim.org. I hope this is of help!

  Marked as helpful

  1

  

  Adrianna Thomas•70

  @adrianna-thomasPosted over 2 years ago

  @emjogale - I really appreciate your feedback, you've been very helpful. I did your suggestions but the mobile view still isn't centered like the desktop view is.

  0

  

  Emma•320

  @emjogalePosted over 2 years ago

  @adrianna-thomas - I think the issue is to do with all the layers you have wrapped the component in. You have declared the body to be a flex container - but this will only affect it's direct children - in this case that is your h1 and the container and so the justify-content and align- items isn't working on the 'wrapper' div. With your existing code - if you declare your container div to be flex and to center justify and align items the card will become centred. It would also work fine if you removed at least one layer - for example you could rename your main section to be main class="container" and remove the container div - but I don't want to confuse you! Just remember that flex containers only control direct children. I hope that makes sense!

  Marked as helpful

  1

  

  Emma•320

  @emjogalePosted over 2 years ago

  @adrianna-thomas one last thing - you will also need to remove the width: 95% from the container div. And you won't need the margins...

  Marked as helpful

  0