@FrederikDiamonD
Papa Elhadj Abdoulaye NDOYE
@NDOY3M4NAll comments
- @NDOY3M4N
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: 100vhon themainthe content is not showing properly. To fix this issue, you could replace that property withmin-height: 100vhso that themainwill only fill up the minimum space needed.You could also add some vertical padding on mobile.
Happy coding 🎉️
Marked as helpful - @wyliemickelson@NDOY3M4N
Wonderful 😍️😍️😍️.
You just need to fix:
- on
1366x750screen, 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 🎉️
- on
- @uspaz@NDOY3M4N
Nice job on this challenge. On mobile you could replace the
height: 100vhtomin-height: 100vhon thecontainerclass. 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 - @mcdulingm@NDOY3M4N
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: 100vhon the main. You could fix this by just replacing it withmin-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 beainstead ofbutton.
Happy coding 🎉️
Marked as helpful - on mobile the padding is not applied to the screen since you set
- @MJ1001@NDOY3M4N
You could add a
max-widthto thecontainerclass to sort-of constraint the width on mobile display. Great work - @imjackfrost1997@NDOY3M4N
congrats, that's a super nice reproduction of the design. Keep at it!
- @NDOY3M4N@NDOY3M4N
Oups, you're right. I totally forgot to change the hover styles on mobile. Thanks for the feedback
- @Andro87@NDOY3M4N
Hi @Andro87, nice reproduction of the design. Maybe you can try adding the hover effect on the cards to complete the challenge.
- @fatihcandev@NDOY3M4N
Yay @fatihcaen congratulations on your implementation of the slider. It looks super smooth. One thing I might suggest is to add an
overflow: hiddenwhen the mobile navbar is open to prevent vertical scrolling.Happy coding :smile:
Marked as helpful - @N1612N@NDOY3M4N
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
- @madizhaksylyk@NDOY3M4N
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.
- @Devcrowmaster@NDOY3M4N
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
- @pavan-areti@NDOY3M4N
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
- @akshaymagrani@NDOY3M4N
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
- @akshaymagrani@NDOY3M4N
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
- @Alicia-M-M@NDOY3M4N
Nice design on the desktop layout but for the mobile layout l, maybe you could put the max-width to 700px or 500px
- @Alicia-M-M@NDOY3M4N
Nice design on the desktop layout but for the mobile layout l, maybe you could put the max-width to 700px or 500px
- @jaseveen@NDOY3M4N
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.
