Skip to content
OverviewSolutions
0
Comments
7
Willem Dubbeldam
@thinkclear67

All comments

  • Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Nice job! My suggestions would be that

    1. take a look at your mobile(landscape & portrait) and tablet version, because something isn't right and the website is not responsive for mobile-landscape and tablet
    2. the original design has more white space
    3. try the mix-blend-mode property for the image instead of opacity
    4. train yourself to code mobile first and adjust for larger screens with media queries if necessary instead of the other way around. This is much easier and less coding. A plain html website (so without css) is already responsive
    5. use the rem unit only for font-size. Margins and paddings are best done in em's, in this way margins and paddings act responsive if the default font-size is not 16px. The same goes for height and width, the vw and vh are better options.

    Keep on coding ;)

    0
  • Mahmoud•185
    @Mahmoud-Elshaer-10Submitted about 3 years ago

    Please provide your feedback. Any suggestions are most welcome!

    HTML CSS

    1
    Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Looks great! My only suggestions would be to look at the size of the buttons in mobile landscape mode and tablet mode and to remove the comments in your HTML document, these are only for developers mode ;)

    Marked as helpful
    1
  • Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Well done! There is only a small problem with the text in the buttons in the mobile version and all cards turn red ;)

    Marked as helpful
    0
  • Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Looks good! Some minor differences on font-size and colour in comparison with the original design, that makes the original looks a bit better. My suggestion would be to use another unit for width and height than rem on the card class. If a user/device uses a different default font-size than 16px, the layout breaks. Note: the font-size in body is declared as 'size' instead of 'font-size'.

    0
  • Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Well done! Mobile first, using CSS custom properties, BEM. It's easy to watch and follow the HTML and related CSS. Personally I would use em's for padding and margin. In that way the margin and padding adjust automatically if you ever decide to change the font-size of an element or if the font-size changes because of different device.

    Marked as helpful
    0
  • ogenathan•50
    @ogenathanSubmitted about 3 years ago

    Please i would appreciate any feedback on how i can improve my skills.

    Html, CSS and JS

    1
    Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Good job. There are some issues with the height and placement of the image. My suggestion would be that you start mobile first and after that adjust with mediaqueries for desktop instead of the other way around. In this way you need to code less. Tip: give the body a margin: 0

    Marked as helpful
    0
  • Willem Dubbeldam•80
    @thinkclear67Posted about 3 years ago

    Looks great. Working with flexbox and em's for padding, well done. My only suggestion would be to define font-size with rem instead of em. Kevin Powell has an excellent Youtube video that explains why using rem for font-size.

    Marked as helpful
    0