Mobile landing page using CSS flexbox

Community feedback

• 

  Fraser Watt•1,790

  @fraserwatPosted almost 3 years ago

  This is looking really nice!! Only things I'd change:

  • Add a bit of border-radius to the image
  • Simplifying the HTML a bit - if you've got the <main> as a container, does it need the <div class="container">?
  • Using semantic HTML. The .attribution component can be a <footer>, what do you reckon the .card component should be?

  Keep it up! Fraser

  Marked as helpful

  0

  ---

  

  De'Andre Barnett•60

  @Babray03Posted almost 3 years ago

  @fraserwat Hey, thanks boss I didn't notice I will make those changes to the border radius, and I will keep the main tag idea in mind for a later project.

  0
• 

  Rai•100

  @RaiIsNotYourGuyPosted almost 3 years ago

  Overall, very good. The border for the image and hover-image are rounded in the sketch-up. Only thing I can see.

  0
• 

  Rio Cantre•9,690

  @RioCantrePosted almost 3 years ago

  Hello there! Awesome job with this project. Viewing your solution, I would suggest the following for you...

  • Add border-radius: 10px; in the .card-img and .card-img__overlay
  • Instead of wrapping the whole part with a to make it a link, it can be simplified with just div and add hover state

  <div class="card-img__container">
          <img src="images/image-equilibrium.jpg" alt="cube" class="card-img">
          <div class="card-img__overlay">
                 <img src="images/icon-view.svg" alt="eye icon">
          </div> 
  </div>
  

  Hope this helps and Keep it up!

  0

  ---

  

  De'Andre Barnett•60

  @Babray03Posted almost 3 years ago

  @RioCantre Hey thanks for the advice, would I add the hover state to my .container class in the css ".container:hover {}" for example?

  0

  

  Rio Cantre•9,690

  @RioCantrePosted almost 3 years ago

  @Babray03 I believe your out of context... If your preferring the hover state of the whole content, then do so. The point I wanted to emphasize is this...

   <a href="#">
          <div class="card-img__container">
          <img src="images/image-equilibrium.jpg" alt="cube" class="card-img">
            
              <div class="card-img__overlay">
            <img src="images/icon-view.svg" alt="eye icon">
          </div>
            
          </div>
          
          </a>
  

  This is the original code in your repo and the one's I showed above was simplified version. If you add another .container it would stack up. Additionally, add border-radius: 10px; in .card-img__overlay .

  I would appreciate it if your mark this as helpful. Cheers!

  Marked as helpful

  0

  

  De'Andre Barnett•60

  @Babray03Posted almost 3 years ago

  @RioCantre okay thanks I will try to add those changes

  1