Responsive NFT preview card component Using HTML5 and CSS3
Design comparison
Solution retrospective
Hello Developers,
I have completed the NFT preview card component. I would love to receive some feedback on the hover state of the card image.
Cheers!
Community feedback
- @pikapikamartPosted almost 3 years ago
Hey, really nice work on this one. Layout in general looks really great.
Some suggestions on the site would be:
- Avoid using
height: 100vh
on a large container like thebody
as this makes the element's height capped based on the viewport/screen's height. Instead usemin-height: 100vh
so that the element will expand if it needs to. - But doing that makes the layout not centered anymore. So add an extra :
align-items: center; display: flex;
On the
body
tag.- Since you are treating the image as an interactive, since there is a hover with something like a preview state right, it would be great to use
button
in here. It would wrap the wholeimg
so that user can properly focus on it and could toggle the preview if they want with just using keyboard. - Sine the
Equilibrium
text is an interactive element, supposing it would be a page-link where user can view the full information about the nft? What is nft :>> It would be great to use :
<h1> <a href="/images/nft">Equilibrium </a> # or just use href="#" for now </h1>
- But to be honest, I wouldn't use
h1
on the nft name in here, theh1
would be a screen-reader only heading inside themain
. Since by looking at this component, on a real site, there would be numbers of different nft? Images in here so using theh1
as a general text would be lot better and using justh2
on the nft? Title. - The
0.041ETH
should be inside ap
tag. Remember that you don't wrap a text directly with adiv
element, always use a more suitable tag for texts. - The clock image is just a decoration on the site. Decorative images should be hidden for screen-reader at all times by using
alt=""
andaria-hidden="true"
to theimg
tag or onlyaria-hidden="true"
if the image is usingsvg
. - Only use descriptive
alt
on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users. - Again, use
p
tag on the 3-days-left. - Person's image should be using the person's name as the
alt
attribute since it is already visible, `alt="Jules Wyvern". - The person's name as well is treated as interactive component so use a interactive element on it like a
a
tag if this would like navigate a user on a different site.
Aside from those, great job again on this one.
Marked as helpful1@michaeljohnson-mjPosted almost 3 years ago@martpika Thanks you so much. You just enlightened me with a lot of stuff which I didn't know. I will definitely try to improvise myself with the feedback you provided.
Your detailed review also taught me how I can review other's code and what one needs to cover during a review. Thanks again.😄
1 - Avoid using
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