Design comparison
Solution retrospective
Used SASS to build this... Hope I did well?
Community feedback
- @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 thea
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. thecolor
andhover
on.card_text h2
and.card_foot p span
Marked as helpful1@jibreel1Posted almost 3 years ago@AndyCormack thanks for this detailed correction... I'll definitely work on everything you said
0 -
- @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 helpful1@jibreel1Posted almost 3 years ago@PhoenixDev22 Noted! Thanks for your help.. I'll definitely work on it
0 -
Please log in to post a comment
Log in with GitHubJoin our Discord community
Join thousands of Frontend Mentor community members taking the challenges, sharing resources, helping each other, and chatting about all things front-end!
Join our Discord