Skip to content
OverviewSolutions
50
Comments
190
Luka Glonti
@lack21

All comments

  • Luka Glonti•3,440
    @lack21Posted about 1 year ago

    Hi there, that's a good work right there, but I have some recommendations!

    • Instead of width: 310px in .container add width: min(500px, 90vw), it will adjust width to 500px and if screen's width will be less than 500px .container will adjust to it!

    • Replace margin: auto in .container to margin: 1.5rem auto to add some additional space to the top and bottom!

    Good Luck! 👍

    Marked as helpful
    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Great work 👍, but I have a suggestion for you!

    • Remove background-size: 100% 100% from class .hero and add these styles instead
    background-size: cover;
background-position: center;

    like this background image will fit well on all devices!

    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Good job 👍, but I have a suggestion!

    • Add background-size: contain to the body, so background image would fit well on larger devices!
    Marked as helpful
    1
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Good job 👍, but I have a suggestion!

    • Instead of padding: 1rem 1.2rem in the class .point use these styles
    width: 3rem;
height: 3rem;

    like this circles will be round!

    Marked as helpful
    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Excellent work 👍, but I have a suggestion!

    • Remove these styles from .container
    margin-top: 100px;
height: auto;

    and add min-height: 100vh instead, like this layout will be centered!

    2
  • levy_m•60
    @LevyMatiasSubmitted over 1 year ago
    • I used the B.E.M methodology in my solution to have better management of my html and css classes.
    • I believe it is not 100% identical. I couldn't expand the background image to cover the width of the screen, so I didn't put it in.
    • Any hints or suggestions I will be grateful.

    Order summary card

    1
    Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Great work 👍, but I have some recommendations!

    • Replace height: 100vh to min-height: 100vh in the body, the difference is that, when you set height: 100vh to something, that means it won't be bigger than that, it might cause some problems in the future, so better approach is to set min-height: 100vh, like this in case content is overflowing container will adjust to it!
    • Remove width: 100% from the body, body is a block level element which by default takes full space available, so width: 100% does nothing here!
    Marked as helpful
    1
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Great job 👍, but I have a suggestion!

    • Remove following styles from the .wrapper
    width: 100%;
height: 100%;

    they do nothing here!

    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago
    This comment was deleted over 1 year ago
    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Excellent work 👍, but I have some suggestions!

    • Replace height: 100vh to min-height: 100vh in the body, the difference is that, when you set height: 100vh to something, that means it won't be bigger than that, it might cause some problems in the future, so better approach is to set min-height: 100vh, like this in case content is overflowing container will adjust to it!
    • Add margin: 0 to the body, this will remove all unnecessary space!
    Marked as helpful
    1
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Great work 👍, but I have some recommendations!

    • Replace height: 100vh to min-height: 100vh in the body, the difference is that, when you set height: 100vh to something, that means it won't be bigger than that, it might cause some problems in the future, so better approach is to set min-height: 100vh, like this in case content is overflowing container will adjust to it!
    • Remove width: 100% from the body, body is a block level element which by default takes full space available, so width: 100% does nothing here!
    Marked as helpful
    0
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Good job 👍, but I have some recommendations!

    • Replace height: 100vh to min-height: 100vh in the body, the difference is that, when you set height: 100vh to something, that means it won't be bigger than that, it might cause some problems in the future, so better approach is to set min-height: 100vh, like this in case content is overflowing container will adjust to it!
    • Remove width: 100% from the body, body is a block level element which by default takes full space available, so width: 100% does nothing here!
    Marked as helpful
    1
  • Luka Glonti•3,440
    @lack21Posted over 1 year ago

    Excellent work 👍, but I have a suggestion!

    • Remove padding-top: 5% from the body and add these styles instead
    min-height: 100vh;
justify-content: center;

    like this layout will be centered!

    0