Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found
Not Found

Submitted

Responsive Web Design built in 0.75 md : Grid, BEM + Passion

#accessibility

@vikramvi

Desktop design screenshot for the Product preview card component coding challenge

This is a solution for...

  • HTML
  • CSS
1newbie
View challenge

Design comparison


SolutionDesign

Solution retrospective


  1. Please review code and let me know if I can improve upon any area of code ?

  2. This project is built in just 3/4 of a working day, but I feel still there is areas of improvement wrt width, height and grid related techniques.

Thanks in advance.

Community feedback

Lucas 👾 104,580

@correlucas

Posted

Hello Vikram Ingleshwar! Congratulations for your new solution!

Your solution seems great. You've done a good job with the GRID and the media queries. There's only two minor details your can adjust to have your solution more accurate in comparison to the design files (starter files).

  1. Note that you've insert the background inside you element with the class .grid-container, its better you insert the background-color <body> to display it filling all the screen. Apply to the body height: 100vh to make sure your background will fill all the screen.

  2. Align your main component applying flex to the body with these properties display: flex; align-items: center; justify-content: center;

See the changes I've done to your code below:

body { display: flex; align-items: center; justify-content: center; height: 100vh; background: var(--clr-primary-cream); }

I hope these tips help you Vikram, then you can say me if these changes fixed the background/position behavior. Happy coding.

Marked as helpful

1

@vikramvi

Posted

@correlucas Hi Lucas,

Thanks a ton for your valuable time and comments.

I've done the fixes ( https://github.com/vikramvi/Product-preview-card-component/commit/4b849dbe9ace1da24c72475c39b59ab5a203325d )

Kind Regards, Vikram

1
Lucas 👾 104,580

@correlucas

Posted

@vikramvi Hello Vikram, thats nice! I just saw your solution and seems to works good! An additional advice is to add some margin in your component to avoid the card touching the screen limits in smaller screens. But anyway now seems much better, congrats.

.grid-container { margin: 20px; }

Happy coding!

0

@vikramvi

Posted

@correlucas I've pinged you on slack, please check

0

@vikramvi

Posted

@correlucas Thanks again for review, do you think instead of adding margin will below work better ?

width: 90%

0

Please log in to post a comment

Log in with GitHub
Discord logo

Join our Discord community

Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!

Join our Discord