Mobile and desktop version, using CSS Grid

Community feedback

• 

  Kyle•80

  @KylesTech95Posted about 2 years ago

  Hi Mariano!

  Great job on this project! I truly appreciate the use of CSS Grid for creating the preview card. Although your CSS look sharp, there is one addition I would like to add to your project in regard to device response.

  -Added below is a media query for the desktop view. The Css on both desktop.css & style.css works out to a wonderful & responsive solution. I would suggest combining the two files with a media query in so users can interchange between the two widths.

    @media (min-width: 1440px) {
      /*Imported desktop.css from github*/
      body {
        width: 100vw;
        height: 100vh;
      }
      
      .card-container {
        width: 38%;
        height: 400px;
        display: grid;
        grid-template-columns: 1fr 1fr;
        grid-template-rows: 1fr 1fr 1fr 1fr 1fr;
      }
      
      .card-container .card-container--header {
        grid-column: 1 / 2;
        grid-row: 1 / 6;
        background-image: url("images/image-product-desktop.jpg");
        height: 100%;
      }
      
      .card-container .card-container-title {
        grid-column: 2 / 3;
        grid-row: 1 / 2;
        font-size: 1.1rem;
        padding: 30px 25px 0 25px;
        margin: 0;
      }
      
      .card-container .card-container-product-name {
        grid-column: 2 / 3;
        grid-row: 2 / 3;
        font-size: 2.8rem;
        line-height: 3rem;
        padding: 0 45px 0 25px;
        margin: 0 0 20px 0;
      }
      
      .card-container .card-container-description {
        grid-column: 2 / 3;
        grid-row: 3 / 4;
        font-size: 1.3rem;
        line-height: 1.8rem;
        padding: 0 25px;
      }
      .card-container .card-container-prices {
        grid-column: 2 / 3;
        grid-row: 4 / 5;
        padding: 0 25px;
        margin: 20px 0 0;
      }
      
      .btn {
        grid-column: 2 / 3;
        grid-row: 5 / 6;
        margin-top: 0px;
        width: 80%;
        height: 45px;
      }
    }
  

  Great work, and I hope this helps!

  Marked as helpful

  0
• 

  Harsh Kumar•1,390

  @thisisharsh7Posted about 2 years ago

  Hey Mariano, Good job! your solution seems perfect but there are some improvements which could be made

  1. html is a language and it tells the page that we are writing html codes , you don't have to give it flex property or any CSS property like here html{ display: flex; justify-content: center; } instead designing of the page should start from body.

  2. At media query min-width: 1440px you can give body{ min-height: 100vh; width: 100vw;} but here avoid media query at this size instead first try to design the page at larger size and then use media query for smaller sizes.

  3. Try to make two separate container one for image and another for text this will help when you give CSS to the page.

  Hope this helps...

  Marked as helpful

  0
• 

  Mariano Tourné•80

  @marianotournePosted about 2 years ago

  Thank you very much for your improvements and suggestions to my solution. I'm going to follow your suggestions.

  Kyle, I separate both css files because I think it's a method of good practices. Harsh, you are correct about the styles in html element. The only I should use in that element is font-size. I took a course that suggested making small screens first and then big ones, that's the reason of my solution.

  0