use grid in all the design

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, awesome work on this one. Layout in genera looks but for mobile state, it is limited to 350px thus hiding the content and creating horizontal scroll on phone with lower width than the layout.

  Danilo Vilhena already gave feedback on this one, just going to add some suggestions 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 the body as this makes the element's height capped based on the viewport/screen's height. Instead use min-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 use main tag on the .card selector.
  • To make the layout responsive even if small phone sizes, instead of using width: 350px on the .card, use max-width: 350px so that the layout will still respond and won't have a fixed width.
  • For the image, I think you should have added that preview-overlay on :hover state so that when hovering on it, it will give a small idea to the user that is is interactive and viewable. If you are going to do this, make sure that there is an interactive element like a button being used to act like the image, making it seem that the image is clickable.
  • The nft image as well should be using a proper alt. Since the nft name is already present, use it as the alt so use alt="Equilibrium".
  • When using img tag, you don't need to add words that relates to "graphic" such as "icon" and others, since img is already an image so no need to describe it as one.
  • For now, you can use h1 for the nft name. Remember that h1 is needed for every page of the site. To be honest on this case, the h1 would be better to be a screen-reader only h1 since on a real site, there would be bunch of other nft as well so it is better to generalized the h1. Have a look at this snippet of mine about screen-reader only h1 I already included comments on it as well^^.
  • Since you are treating the nft name as clickable since there is a hover state on it, use a tag on the nft name and wrap it by heading tag:

  <heading tag>
    < a href="/nft/equilibrium">Equilibrium #3429 </a>
  </heading tag>
  

  Whenever you make something interactive, always use interactive elements.

  • Use only ethereum as the alt, remove the icon word.
  • The clock-icon is only decorative on this. Decorative images should be hidden for screen-reader at all times by using alt="" and aria-hidden="true" to the img tag or only aria-hidden="true" if the image is using svg.
  • Only use descriptive alt on images that are meaningful and adds content to the site otherwise hide the image for screen-reader users.
  • The person's name is already present so it will be better to use their name as the person's image alt.

  Aside from those, great job again on this one.

  Marked as helpful

  1

  ---

  

  Dhiya Kellouche•50

  @proXDhiyaPosted almost 3 years ago

  @martpika thanks for your feedback, it really helps

  1
• 

  Danilo Vilhena•160

  @danilovilhenaPosted almost 3 years ago

  It looks good! I just have a few things to point out:

  1. The main image doesn't have border radius
  2. The card box shadow is a bit too dark at the bottom
  3. The cyan overlay (with the eye) on top of image should be on hover, not on click
  4. The spacing is just a bit different from the design

  But those are just details, overall you've submitted a great solution!

  Marked as helpful

  1

  ---

  

  Dhiya Kellouche•50

  @proXDhiyaPosted almost 3 years ago

  @danilovilhena thank you ^-^

  0