Skip to content
Submitted over 2 years ago

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

Vikram Ingleshwar•280
@vikramvi
Preview site (opens in new tab)View code (opens in new tab)
This is a solution for...Product preview card component
  • HTML
  • CSS
1newbie

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,340
    @correlucasPosted over 2 years ago

    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
    Vikram Ingleshwar•280
    @vikramviPosted over 2 years ago

    @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,340
    @correlucasPosted over 2 years ago

    @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
    Vikram Ingleshwar•280
    @vikramviPosted over 2 years ago

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

    0
    Vikram Ingleshwar•280
    @vikramviPosted over 2 years ago

    @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

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