@PhoenixDev22
Posted
Hello @rahunak ,
Congratulation on completing this frontend mentor challenge.
I have some suggestions regarding your solution:
-
First of all , you have used some extra
<div>
.Remove the commented code that may confuses other developers or even you in the future. -
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 . -
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
. -
the
icon-view
doesn'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. -
There are so many ways to add the hover effect on the image , The one I would use, using 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.
The link wrapping the equilibrium image should either have Sr-only
text, an aria-label
or alt
text that says where that link takes you.
-
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-view, icon-ethereum, icon-clock
). -
Never use
<span>
alone for meaningful content , so you can use<p>
instead. -
To use more semantic tags , you can use <ul> to wrap
class="coin_description__count_img"
and in each<li>
there would be<img>
and<p>
. -
The avatar's alt should not be
"image-avatar"
It's meaningless , you can use the creator's nameJules Wyvern
. Read more how to write an alt text
CSS
First of all why display none for the smaller screens ?
`@media (max-width: 374px) {
body {
display: none;
}
}
-
As you used flexbox properties to the body you can add min-height to the body to center the component on the middle of the page .
-
width: 335px;
an explicit width is not a good way . consider usingmax-width
to card instead and a little margin to the card .That will let it shrink a little when it needs to
General points :
-
Remember a css reset on every project. That will do things like set the images to display block and make all browsers display elements the same.
-
It's recommended to use
em
andrem
units .Bothem
andrem
are flexible, Usingpx
won't allow the user to control the font size based on their needs.
Overall , your solution is good. Hopefully this feedback helps.
Marked as helpful
@rahunak
Posted
@PhoenixDev22 Thanks for your advice, i correct my code. You are really smart and good human.Thanks you a lot.