Frontend Mentor | Mobile landing page using CSS flexbox coding challenge solution

Community feedback

Fraser Watt• 1,790

@fraserwat

Posted over 2 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

De'Andre Barnett• 60

@Babray03

Posted over 2 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.

Rai• 100

@RaiIsNotYourGuy

Posted over 2 years ago

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

Rio Cantre• 9,710

@RioCantre

Posted over 2 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!

De'Andre Barnett• 60

@Babray03

Posted over 2 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?

Rio Cantre• 9,710

@RioCantre

Posted over 2 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

De'Andre Barnett• 60

@Babray03

Posted over 2 years ago

@RioCantre okay thanks I will try to add those changes

Mobile landing page using CSS flexbox

Design comparison

Community feedback

Please log in to post a comment

Join our Discord community