NFT card component built with SCSS

Community feedback

• 

  P

  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

  ---

  

  Jibreel•300

  @jibreel1Posted almost 3 years ago

  @AndyCormack thanks for this detailed correction... I'll definitely work on everything you said

  0
• 

  PhoenixDev22•16,950

  @PhoenixDev22Posted almost 3 years ago

  Hello Jibreel, Congratulation on completing your project. I have few suggestions :

  To tackle the accessibility issues :

  • Document should have one main landmark. Wrap the body content in< main>tag read more about main landmark.

  • Page should contain a level-one heading. You can replace the <h2> by <h1>.It's essential for the heading to go in order, SO you can replace the <h3> by <h2>.

  • <a href="#"><h2>Equilibrium #3429</h2></a>is invalid html to wrap a heading element in an anchor tag. Swap those around so the anchor is inside the heading. -You need to add an interactive element around the image . Anything with a hover style in a design means it's interactive. -For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. (<img src="/images/icon-clock.svg" alt="clock" /> and <img src="/images/icon-ethereum.svg" alt="eth" />) .

  • You can replace the <div class="attribution">by a <footer > and it would be out the < main>. Last I would suggest to work on box-shadow to look close to the challenge. I really tis feedback helps , happy coding

  Marked as helpful

  1

  ---

  

  Jibreel•300

  @jibreel1Posted almost 3 years ago

  @PhoenixDev22 Noted! Thanks for your help.. I'll definitely work on it

  0