@PhoenixDev22
Posted
Hello @Beats-Ayush,
I have some suggestions regarding your solution:
-
There should be two landmark components as children of the body element - a
main(which will be the NFT card ) and afooter(which will be the attribution).<Footer>should not be in the<main >. HTML5 landmark elements are used to improve navigation . -
Never use
<div>alone for meaningful content . You can use a header . In this challenge , you can use<h1>OR you can use<h2>and have<h1>withclass="sr-only"(Hidden visually, but present for assistive tech). -
For any decorative images, each img tag should have empty
alt=""andaria-hidden="true"attributes to make all web assistive technologies such as screen reader ignore those images in(icon-ethereum, icon-clock). -
the
icon-viewdoesn't really need to be in the html, you could do it with css. If you want it to stay in html it needs to be aria-hidden or role presentation with empty alt. -
I would use pseudo-elements to change the teal bg color to a hsla. Then opacity can be changed from 0 to 1 on the pseudo element on hover as there is no reason to have the extra clutter in the html.
-
Anything with a hover style in design means it's interactive. you need to add an interactive element
<a>around the image,Equilibrium #3429 and Jules Wyvern. add hover effect on them . -
The avatar's alt shouldn't be
empty, you can useJules Wyvernfor it. -
the link should be wrapping the main image and either have
Sr-onlytext, anaria-labeloralttext that says where that link takes you. -
You are misusing the
<section>tag . section is not meant to be used anytime you feel tempted to use a div . section is for a bigger chunk of content often titled by<h2>Read more about usage notes. -
So you can use
<ul>to wrapclass="txt__body--icons"and in each<li>there would be<img>and <p>. Also never use<span> `` alone for meaningful content . -
You should use
emandremunits .Bothemandremare flexible, Usingpxwon't allow the user to control the font size based on their needs. Never use px for font-size. -
What's the purpose of these?
<div class="container-one"></div>
<div class="container-two"></div>
- To center the card on the middle of the page , you can use the flexbox properties and
min-height: 100vhfor the<body>like this :
body{
display:flex;
align-items: center;
justify-content: center;
width: 100%;
min-height: 100vh
-
width: 300px;an explicit width is not a good way . Remove the width from the main component and change it tomax widthinstead. That will let it shrink a little when it needs to. -
font-size: 62.5%;this has huge accessibility implications for those of us with different font size or zoom requirements. It's recommended not change the html or root font size.
Overall, your solution is good, Hopefully this feedback helps.
Marked as helpful
@Beats-Ayush
Posted
@PhoenixDev22 Thanks so much for the extremely informative feedback.
I used the container1 and container2 for the slight darker backgrounds behind the NFT card. I thought it is something significant but after going through some of the other solutions, it isn't apparently.
Also what's meant by Also never use <span> `` alone for meaningful content .?
Thanks again for your code. I'll study it and improve.
