Skip to content
OverviewSolutions
28
Comments
16
Benja.min
@BenjaDotMin

All comments

  • Benja.min•740
    @BenjaDotMin

    Hello Naomi! Apologies if I read this wrong, but we can very easily center your card, and remove the scrolling.

    On your .container tag, remove: margin: 25% auto 25% auto; (also the media query margin at 510). On your body tag add: display: grid; place-items: center; height: 100vh;

    And that should center things up nicely :) Great work, you have made the card itself very clean.

  • Benja.min•740
    @BenjaDotMin

    Hello! I see you have very precise margins to centre your card. We could make this far simpler by removing these styles from .qr-container:

    • display flex
    • flex-direction
    • height (both of them)
    • margin
    • align items
    • margin-top (and the media query at 1020)
    • margin-bottom

    Then to your body tag add:

    • display: grid
    • place-items: center
    • height: 100vh

    That should centre your card with less code, but also remove the unrequired scrollbar on mobiles. Hope that helps!

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    I really like what you did with the validation animation. Subtle but it really helps add quality. Would you mind if I suggested you add:

    • .notify__field{z-index:1;}

    That will stop your error text overlapping the input during animation, and look even better :)

    Good stuff :)

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Apologies for the preview screen, I assume its something to do with lazy loading the components in react. Pop open the site and it should be fine!

  • Benja.min•740
    @BenjaDotMin

    Hello there!

    You really do have a great grasp with grid, for your first attempt. Much better than I did!

    However, regarding your question:

    • To your body tag add: height:100vh; display: grid; place-items: center;
    • To your main tag add: max-width: 1440px;

    That should improve the size and positions. You can play around with values to match what you need.

    It wont make a difference, but consider the shorthand syntax for grid-template, to save yourself repeating values and make it easier to read.

    For example: grid-template-columns: 1fr 1fr 1fr 1fr; grid-template-rows: 1fr 1fr;

    Could simply become: grid-template: repeat(2, 1fr) / repeat(4, 1fr);

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Hello Paras!

    The issue is when you hover over the thumbnails, they get an additional border, which changes the height of the parent div by an additional 2px, giving the visual stagger.

    To fix this I would:

    • on ".img-row img" add: border: 2px solid transparent; (so they always have a border)
    • on ".img-row img:hover" change your border style, to: border-color: hsl(26, 100%, 55%);

    Hope that helps!

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Hiya Martijn!

    You can add a z-index:1 to .primary-btn. Then add z-index: -1 to your after class.

    With a z-index and relative parent, the z-index:1 will act as a new scope, for the z-index:-1 (rather than the -1 sending it behind your page, it will send it behind the primary-btn) I hope this makes sense :)

  • Benja.min•740
    @BenjaDotMin

    Hello!

    I would go for a display flex approach, rather than floats.

    • Remove the padding-top from .logo
    • Add to .logo, .eth and .time: display: flex; align-items: center;

    This should centre them up nicely.

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Heya Chris,

    Add: .imageContainer .mainImage{display:block;} to your css, and this should fix your issue :)

    Images are display inline by default, so it always causes some weirdness with vertical spacing, as the box model doesn't tend to support top/bottom with inline elements.

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Hello Walid!

    • Remove margin-top:50px from .container
    • Add to body: display: grid; place-items: center; min-height: 100vh;
    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Hello Adina! First off, great work with this, I think you have handled the spacing very well. Looking great.

    Regarding your question, here is how I would handle the eye hover.

    • Delete ".viewicon" img tag inside ".overlay"
    • Delete the ".viewicon" css
    • Change opacity:0.5 on ".containerimage:hover .overlay", to opacity:1
    • On your ".overlay" class, add: background: url(./images/icon-view.svg) no-repeat center rgb(0 255 247 / 50%);

    This should centre the eye, but also give you a transparent background. Apologies, I do not know what the exact colours are from the design, so I just guessed. You may be able to get it closer.

    The issue you were having is due to that opacity is inherited by all children, whereas rgba is not inherited by children.

    Also I notice on your ".overlay" class, you have: transform: 0.3s ease; This should be: transition: 0.3s ease; That should get your animation working.

    This is all I have, but again good job!

    Marked as helpful
  • Benja.min•740
    @BenjaDotMin

    Excellent points there, thank you so much for the time and detail. I will bear all these in mind for the future.

  • Benja.min•740
    @BenjaDotMin

    Thanks for your time Mikev, I will amend those points.

  • Benja.min•740
    @BenjaDotMin

    Hello! Apologies, I am not experienced enough to answer your second question, but I can give insight into your first question.

    When it comes to flexbox and grid (especially grid) I would move away from percentages, and in some cases pixels too. If you examine the width of each car section, you will see they are very slightly different widths, because % is not super accurate in this case. However, if you remove your min-widths, and change your car sections (.sedans, .suvs, .luxury) each to flex:1; then that will make them all perfectly even width, and be repsonsive.

    If you wanted differing widths, you could set one of the car sections to flex:1.2; for example (but not needed in this design). Flex works out the width relative to each other, in the given parent. So flex:2, would be twice the size of an item with flex:1, etc.

    If you want to get into using Grid, I would also recommend using "fr", due to %'s wont work correctly along side the gap rule. Sorry if this is longwinded, I just wanted to give better options than %, or static pixels.

    Also, just as a side, if you really want to use %'s, to get more accurate widths, you would need to used something like 33.333%, which really is not pretty and will give you very specific widths like 399.999px wide due to the missing .111%. Where as flex and fr will be perfect.

  • Benja.min•740
    @BenjaDotMin

    Hiya David, great work here.

    Regarding your question: I would put on your body tag: display: flex; align-items: center; justify-content: center; height: 100vh;

    Then on your main tag: max-width: 920px; and remove the margin rules you have for desktop and mobile.

    Then on your footer tag: position:fixed; bottom: 40px... or so.

    That should centre things up nicely!

  • Benja.min•740
    @BenjaDotMin

    First of all, great work! But one small tip about the button hover state. Set your button border to 2px white (remove the hovered border), then in your hover state, just set the background of the button to transparent... that way the button will not jump slightly when hovered, as the border never changes :)

    Hope that's helpful!