Skip to content
OverviewSolutions
12
Comments
3
Samson Sham
@samsonsham

All comments

  • Samson Sham•140
    @samsonsham

    Hello Facundo 👋 Congratulations for completing this challenge 🎉 I see you make good use of grid box and calculate the row and column nicely👍

    Some of my suggestions below:

    • I see you have imported the font family Poppins but I couldn't see you are using it. font-family: 'Poppins', sans-serif; So you might not be intentionally not using the given font, right?
    • I see you put the first part of heading into <span>. It should be part of the heading so is semantically better to put it into h1 as well and add necessary <strong> and line break. And <span> is an inline element so be careful your using might cause some unwanted inline effect.
    • Incorrect icon in Calculator card.

    Hope this helps! Have a nice day!

    Marked as helpful
  • Samson Sham•140
    @samsonsham

    Hello Eric 👋 Congratulations for finishing this challenge🎉 Your solution looks nice!

    When I was doing this challenge, I also noticed there was only one svg for the share button. That was the non-active state. This could mean that if we want an active state image, we have to some how massage the image by ourselves before using it.

    From the design we know the active state share icon is white in colour. I found an answer in this discussion by using filter: brightness(0) invert(1); to do the trick.

    How it works: It first change your svg to black by brightness(0) and then change it to white by invert(1).

    Hope it helps 💪

  • Samson Sham•140
    @samsonsham

    Hello Kally!👋 You did a great job on this challenge🎉 The responsive design is nice, especially the tablet view. I like it very much.👍

    For the scrolling, I found it caused by the margin in .container that you are applying 5% on margin top and bottom. I am afraid if it would be too much for the total page height. As percentage is relative unit and it would be a bit hard to calculate and control if you put it to height. You may reset it to 0 then try some other ways.

    • A quick and dirty way is to add overflow: hidden; to body so no more scrolling (remember to undo it in mobile view otherwise you can't scroll in mobile).
    • If you only want some head space you might only add padding-top or margin-top to avoid unwanted bottom space.
    • If you want to make your container align centre vertically, you can set display: flex; align-items:center; in body.

    Hope it would help! 💪