Responsive landing page using css grid

Community feedback

• 

  P

  visualdennis•8,375

  @visualdennissPosted over 1 year ago

  Good job on completing the challenge successfully! Your solution looks great overall. Especially on desktop.

  I noticed in mobile view, the column have very high height. You could reduce that by using max-height: some-pixel-value( e.g. 600px), but also adjust the space between elements. It looks like you are using 40% margin for the button, i'd say 40% causes issues, as it is in relation to the containers height, so the bigger the container is margin will be higher, but this is not what you want in this case. So you can replace that with a fixed value like 40px or some 'em' etc. to make it consistent.

  Hope you find this feedback helpful!

  Marked as helpful

  1

  ---

  

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  @visualdenniss never ever set max height or height on elements containing text. All that does is introduce potential for breakage when users change their settings or editors adjust the content. It’s really important to keep solutions flexible.

  1

  

  P

  visualdennis•8,375

  @visualdennissPosted over 1 year ago

  @grace-snow Ok, thanks for the reminder and the tip. Will keep that in mind!

  0
• 

  T

  Grace•31,230

  @grace-snowPosted over 1 year ago

  I recommend you try this again without bootstrap. It’s a simple component and while learning libraries like bootstrap will only slow your genuine learning of the fundamental technologies you need to master (accessible html, css and js)

  Some feedback I hope helps you:

  1. Only have one h1 per page. This is a single component challenge, not a full web page so you don’t need to include any h1 at all. If this was a real page these cards would have h2s
  2. Buttons and anchor elements have distinct purposes. Learn more would navigate to more information about that car. Navigation means it must be an anchor (link) not a button
  3. You never need to use 100vw. Main is already full width anyway
  4. Never ever write font size in px. Extremely important. Use rem so that user preferences can be honoured
  5. Using % for things like margin and padding is not a good approach. You lose all control. Em can be good for vertical margins so it relates to the elements font size and can create good vertical flow/rhythm. Or rem is good if you want to have more control
  6. There is no reason to set things like width 90%. Again you are losing all control doing that. Instead, use things like max-width in rem or ch units for text elements
  7. Icons like those cars can have an explicit width and height. In fact, icons are one of the very few times that is recommended
  8. You can set border radius once on the whole component and use overflow hidden to crop its children’s background colors from the corners. Much simpler than setting individual radius on the children
  9. Do not limit the height of text containing components like this (as already said on the other comment)
  10. Next time work mobile first and make sure your media query definition is written in em or rem, not px

  2

  ---

  

  Shreyansh Swarnkar•70

  @Shreyansh952Posted over 1 year ago

  @grace-snow Thank you so much for this. I will get on it right away.

  0
• 

  Shreyansh Swarnkar•70

  @Shreyansh952Posted over 1 year ago

  the solution screenshot does not match the actual webpage.

  0