@pikapikamart
Posted
Hey, great work on this one. Right now, the overall layout looks huge both desktop and mobile state.
Krisztina Kovari already gave really great feedback on this and I second to all her feedback. Here are some suggestions for the site as well:
- Don't use
width: 100vw
since this will only add a horizontal scrollbar at the bottom, since this value does not account the vertical scrollbar's width. - Avoid using
height: 100vh
on a large container like thediv
that holds the nft-card 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. - Always have a
main
element to wrap the main content of the site. For this usemain
tag on thediv
after the#root
selector. This way, your content will be inside a main landmark. - Like what Krisztina had said, the
:hover
state for the preview image is not created. Just a quick suggestion, if you are going to implement the:hover
state make sure that you are going to use an interactive element that could be nesting theimg
. Because as you may remember, animg
tag is not an interactive element. You could use eithera
tag orbutton
, depending on whatever you think clicking the image would do. - For the nft image, you could use the nft name as the
alt
value since it is already present. You could use something likealt="nft Equilibrium #3429"
. - For the nft name, since it is an interactive component, use an
a
tag inside theh1
so that it will be interacted with. - For the eth-icon and the clock-icon, you could use
alt=""
andaria-hidden="true"
on both of it since on this one, it is acting as decorative images. Decorative images are just images that doesn't contribute to the overall content of the site. - Also, when using
img
tag, you don't need to add words that relates to "graphic" such as "icon" and others, sinceimg
is already an image so no need to describe it as one. - For the person's
img,
you could use the person's name as well since it makes sense to do so since on this layout, it is showing the creator of the nft. - Change the
span
inside the person's name to usea
tag so that it will be interactive. - Lastly, just re-styling the whole card or the size of it so that it will closely match the design's dimension.
Aside from those, great job again on this one.
Marked as helpful
@rebeccaaaaaaaaaaa
Posted
@pikapikamart Thanks Raymart for the comment, it was correct all these points and I'll be back soon with the most updated version of the project.