@pikapikamart
Posted
Hey, nice work on this one. Layout in general looks great, just needed for it to be centered vertically. For responsiveness, it responds but when you go to like 374px, the screen-starts to hide the content and creates horizontal scrollbar.
Leonardo de Souza Nunes already gave great feedback on this one, just going to add some suggestions as well:
- It would be great to have a base styling of this:
html {
box-sizing: border-box;
font-size: 100%;
}
body {
margin: 0;
}
*,
*::before,
*::after {
box-sizing: inherit
}
This way, handling an element specially its size will be easier because of the box-sizing
- Always have a
main
element to wrap the main content of the site. For this usemain
tag on the.main
selector. - Using
margin
alone is not consistent when centering a layout, especially when you need to center it vertically. On this one, instead of that , remove themargin
on the.main
and on thebody
tag, you could add these stylings:
align-items: center;
display: flex;
justify-content: center;
min-height: 100vh;
If you don't know these stylings, have a search about flexbox
^^
- Since the nft image name is already present on the site, it would be nice to use the name as the
img
tag'salt
value likealt="Equilibrium"
. - Also for the hover-overlay. As of now, I haven't seen a solution that uses interactive element on this which should be present. I don't have a reference on this since I haven't made this challenge yet but let me just give a quick idea on approaching element in general.
When you are doing a component, let's say this one, the image. Since on the design, if you are hovering on it, it would create an overlay and like have a preview-icon right. When an component is being treated as an interactive component, always make sure that you are using an interactive element like button
or a
tag. Because lots of submission, they are only adding the :hover
state on the img
tag but there is no interactive element being used. For this, imagine if you click the img
, what would happen? Does clicking it would take a user in another page to view the full nft, if yes, then a
tag will be better to use. But if clicking the img
will only like show a modal/pop-up to see the full image and not redirect a user in another page, then using button
will be your choice.
- Also for the nft name, since you are treating it as interactive as well, make sure that it uses interactive element. You could use
a
tag to wrap the nft name:
<h1>
<a href="#" >Equilibrium #3429</a>
</h1>
This way, it is proper.
- Change those 3
section
tags on the eth, clock and the person part.section
tags is not informative as a landmark unless they are used witharia-labelledby
attribute. You could usesection
if it already have a visible heading tag. But for this, using adiv
would be fine. - When you think an
img
is decorative, adding an extraaria-hidden="true"
attribute on theimg
would be really nice, along with thealt="'
. This way, screen-reader won't picked those up since they are being hidden from it. - Person's image should be using the person's name as the
alt
value likealt="Jules Wyvern"
. Components like this where a person's information is presented, make sure to use their name as the person'simg
alt
value. - Also, since the person's name is again, being treated as interactive, adding using
a
tag rather thanspan
will be much better.
Aside from those , great job again on this one. Let me know if you have queries and see if I can help with it.
Marked as helpful