Skip to content
OverviewSolutions
37
Comments
19
Papa Elhadj Abdoulaye NDOYE
@NDOY3M4N

All comments

  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 2 years ago

    Hi there. Great work on this challenge. I think you used the features (nesting mostly) correctly. Now there are some things in the design that you forgot.

    On mobile, because of the height: 100vh on the main the content is not showing properly. To fix this issue, you could replace that property with min-height: 100vh so that the main will only fill up the minimum space needed.

    You could also add some vertical padding on mobile.

    Happy coding 🎉️

    Marked as helpful
    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 2 years ago

    Wonderful 😍️😍️😍️.

    You just need to fix:

    • on 1366x750 screen, the modal is not showing properly (the close button is off the page), you could reduce the size of the carousel to accommodate for that.
    • on desktop, when hovering on the links the whole navbar is slightly moving. You could add the border with a transparent color on the default state, when in hover state you just change the color of the border.
    • you could also allow the users to open the navbar on mobile

    Happy coding 🎉️

    0
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 2 years ago

    Nice job on this challenge. On mobile you could replace the height: 100vh to min-height: 100vh on the container class. You could also add some vertical padding on mobile. Other than that I think everything is okay. @Lucas feedback is also great. I use a similar technique when doing this challenge. Happy coding 🎉️

    Marked as helpful
    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 2 years ago

    Hi, great work on this challenge. There are just some things that need fixing like:

    • on mobile the padding is not applied to the screen since you set height: 100vh on the main. You could fix this by just replacing it with min-height: 100vh.
    • on mobile you could also increase the padding (like in the design)
    • on line 37 (index.html) there is a typo, the closing tag should be a instead of button.

    Happy coding 🎉️

    Marked as helpful
    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 3 years ago

    You could add a max-width to the container class to sort-of constraint the width on mobile display. Great work

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted about 4 years ago

    congrats, that's a super nice reproduction of the design. Keep at it!

    0
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 4 years ago

    Oups, you're right. I totally forgot to change the hover styles on mobile. Thanks for the feedback

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 4 years ago

    Hi @Andro87, nice reproduction of the design. Maybe you can try adding the hover effect on the cards to complete the challenge.

    0
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted over 4 years ago

    Yay @fatihcaen congratulations on your implementation of the slider. It looks super smooth. One thing I might suggest is to add an overflow: hidden when the mobile navbar is open to prevent vertical scrolling.

    Happy coding :smile:

    Marked as helpful
    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Nice reproduction of the design. For the mobile view, you could set the max-width to 500px or 700px instead of 300px to cover the range of most of the mobile devices

    0
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Hi, nice approach with the positioning but there is a lot of repetition in your CSS. Also it seems like you forgot the mobile approach. You could start with the mobile approach and then adapt your code for the desktop view.

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Yo! You did a great job at following the designs in the mobile and desktop layout. Bravo! In your desktop layout, maybe you should set the background-size to cover in desktop and 100% in mobile

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Hello friend. You did a great job in the desktop design. You just forget to add a margin-top at your document. In mobile, the element are not stacked one on another, I think it’s because of the img size (width: 100%). Also in your media query I don’t think you need a grid-template-columns if you have a grid-template-areas defined

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Hello, nice design on desktop. But I think you should rethink your mobile design, specially in the definition of the media-queries. Also there is a lot of repetition in your css. You have to adopt a DRY approach

    0
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Hello, nice design on desktop. But I think you should rethink your mobile design, specially in the definition of the media-queries. Also there is a lot of repetition in your css. You have to adopt a DRY approach

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Nice design on the desktop layout but for the mobile layout l, maybe you could put the max-width to 700px or 500px

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Nice design on the desktop layout but for the mobile layout l, maybe you could put the max-width to 700px or 500px

    1
  • Papa Elhadj Abdoulaye NDOYE•770
    @NDOY3M4NPosted almost 5 years ago

    Hi there! I think you did great in the desktop layout but in the mobile layout, you forgot to add some margins and padding to your code.

    1