Responsive

Community feedback

• 

  Elaine•11,400

  @elaineleungPosted about 2 years ago

  Hi Paschal, since Lucas gave you some excellent advice already, I'll follow him with some comments on the use of section:

  Right now, you have a section within another section. Nesting sections is not exactly considered good practice, as sections should typically be at the same level with one another. Also, when using section`, it should contain a heading. If it doesn't have a heading, then perhaps it should be just a regular div. I suggest changing your HTML structure to something like this:

  <main>
     <div class="card-container">
         <div class="image"></div>
         <div class="text"></div>
     </div>
  </main>
  
  // you can also try <article> for <div class="card-container">
  

  I also suggest that your CSS can be structured in a cleaner way as it's a bit hard to read right now, and lastly, I'd reduce the margins within the elements in the text container, as right now it's creating a lot of unnecessary space. Instead having margins all around those elements, try only either top or bottom margin only.

  Great work on the whole 😊

  0
• 

  Lucas 👾•104,340

  @correlucasPosted about 2 years ago

  👾Hello Paschal, congratulations for your new solution!

  🎯 Your solution its almost done and I’ve some tips to help you to improve it:

  Instead of using HEADER you can manage both images inside the <picture> tag and use the html to code to set when the images should change setting the device max-width depending of the device (phone / computer) Here’s a guide about how to use picture: https://www.w3schools.com/tags/tag_picture.asp

  A best practice to have all the image inside the container scaling and respecting the size of the container, you need to add max-width: 100% and add alsoobject-fit: cover to make the image auto-crop when resizing inside the column:

  img {
      display: block;
      object-fit: cover;
      max-width: 100%;
  }
  

  ✌️ I hope this helps you and happy coding!

  0