Skip to content
OverviewSolutions
0
Comments
3
P
Andy Cormack
@AndyCormack

All comments

  • Andy Cormack•60
    @AndyCormackPosted over 2 years ago

    Hi Jibreel!

    Great job on matching the design so well.

    Regarding the issues you've been having with the background pattern, it looks like you've made it a little complicated for yourself. If you remove the background-image from body and remove the body::before pseudo-element entirely you can simply add the following to the div that contains the left side content:

    background-image: url(/images/bg-pattern-desktop.svg);
background-size: cover;
    Marked as helpful
    0
  • Andy Cormack•60
    @AndyCormackPosted almost 3 years ago

    Hi Jibreel! Nice work on the challenge! Looks excellent 👍

    Some suggestions and comments:

    Colour accuracy looks a little off from the designs, did you download the design files to get the exact colour values? You can open up the Figma one easily by creating an account on https://www.figma.com/ which will allow you to inspect the colour values of the elements in the design via the sidebar. For example, the card background colour should be #15263F but yours is #14253D I recommend you have a look through and correct for those if you want to match the design more closely.

    The same can be said for minor details like the border-radius on the card corners, which are 15px on the design vs your 10px and the box-shadow which is 0px 25px 50px rgb(0 0 0 / 10%) in the design. Definitely worth having a look through the design on Figma.

    For the code itself, a couple of minor suggestions here:

    • Try and organise your styles slightly better, currently you have a variables and a globals file, the globals having effectively all styles in it. My suggestion would be to split base element styling such as html, body, p, a, and h1 etc. keeping those in your globals file, and then styling the card in the style.scss file itself.

    • Try not to nest your styles too heavily, it's an easy trap to fall into with SASS but one you should try and minimise where you can. The biggest example of this is the .card_price section with .price1 and .price2. You can also combine some of these selectors for styles that are common, helping you keep things that should be identically styled together better, e.g.

    .price1,
.price2 {
    display: flex;
    align-items: center;

    h3 {
        margin-left: 5px;
        font-size: 0.8125rem;
    }
}

.price1 {
    h3 {
        font-weight: 400;
        color: $cyan;
    }
}

.price2 {
    justify-content: flex-end;

    h3 {
        font-weight: 300;
        color: $softBlue;
    }
}

    You might also consider the naming of these classes, as they are slightly misleading and may lead to confusion if you come back later to update the project. My suggestion is to rename .card_price to .card_data, then .price1 can become .card_price and .price2 can become .card_time. Small things like this may seem insignificant now, but the larger a codebase grows the harder these things get to maintain later!

    You can also save yourself quite a few style overrides if you set your a styling to do be white with cyan on hover. If you remove the .attribution part of your styles wrapping the a on line 18 here: https://github.com/jibreel1/nft-card-component/blob/master/sass/_globals.scss#L18 you can then drop the styles to override elsewhere, e.g. the color and hover on .card_text h2 and .card_foot p span

    Marked as helpful
    1
  • Andy Cormack•60
    @AndyCormackPosted almost 3 years ago

    Hi Shannon!

    Great work on this! Looks really close to the design!

    I have a few notes you might want to consider:

    Currently the letter-spacing on your h1 is a little wide compared to the design, this is arguably down to personal preference, but if your goal is to match the design as much as possible, I'd recommend styling it to letter-spacing: -0.05em

    The ratings section on the right hand side you can do similarly to how I suggested you tackle the reviews section steps by using the :nth-child pseudo class selector.

    You also might be better off approaching the styling for the alignment slightly differently, you can centre align all three of them with a margin-left and margin-right of auto (simplified into a shorthand style property of margin: 15px auto), and then simply give the first and third elements margin-left: 15px and margin-right: 15px respectively. This allows the alignments to be less rigid when the width of the screen changes.

    The ratings stars are also the default colour from the icon font source, to match the design make sure you override the color for them by targeting rating-card fa-star.

    Make sure you check through and resolve the accessibility issues listed in the report on this page! Some helpful information in there on how and why you should be making the changes suggested.

    The one other key thing to think about is the solution on mobile devices and screen sizes! Remember to use @media to style for certain breakpoints https://developer.mozilla.org/en-US/docs/Web/CSS/@media

    Also, don't forget to set your own name in the footer and link to your GitHub page or website!

    Marked as helpful
    0