html & css

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted almost 3 years ago

  Hey, nice work on this one. Layout in general looks great, for mobile state, if you go to like 375px below, you will notice that the content is being hidden by the screen.

  MikevPeeren already gave great tip on this one, just going to add some suggestions on the site as well:

  • Avoid using id to target and style an element since it is a bad practice due to css specificity. Instead, just use class to target element.
  • Use footer for the .attribution selector so that it will be its own landmark.
  • Also, if you try zooming out on your screen, you will notice that the layout is not being centered properly( on y-axis). Using margin only is not consistent, what you can do is that remove the margin on the main tag, then on the body tag add these stylings:

  align-items: center;
  display: flex;
  flex-direction: column;
  justify-content: center;
  min-height: 100vh;
  

  This will make sure that content will be centered. Though add a margin-bottom on the main tag or just use gap inside the body tag.

  • When using img tag, you don't need to add words that relates to "graphic" such as "image" and others, since img is already an image so no need to describe it as one. For the nft image, since the nft name is already present, use it as the value like alt="Equilibrium".
  • On a site, always have a single h1. Since there are no visible text that are suitable to be h1, the h1 would be a screen-reader only heading. Meaning it will be hidden visually but present for screen-reader users. On this, the h1 would have like sr-only class and the text-content should describe what is the main-content is all about. The h1 could be placed as the first text inside the main.Have a look at this snippet of mine about screen-reader h1. If you have query about this one, just let me know okay.
  • Add an extra aria-hidden="true" for images that are using alt="" so that they will be totally hidden for screen-reader user.
  • When wrapping up a text-content, make sure that it is inside a meaningful element like p tag or heading tag and not using like div, span to wrap the text.
  • Person's image should be using the person's name as the alt value like alt="Jules Wyvers". Components like this where a person's information is presented, make sure to use their name as the person's img alt value.
  • Lastly, to resolve the issue about the 375px below, on your main tag, just remove the width: 380px since this will just add a fixed width, you can use max-width instead of width.

  Aside from those, great job again on this one.

  Marked as helpful

  1
• 

  MikevPeeren•2,100

  @MikevPeerenPosted almost 3 years ago

  Hey 👋

  Good job finishing this one 👏

  Try to use h1 before you use h2, also try to add more descriptive alt text when you do add it so screen readers know what they are viewing. I am also missing the hover effects 😢

  Marked as helpful

  1

  ---

  

  Abdelilah•180

  @abdeldevprojectsPosted almost 3 years ago

  @MikevPeeren oh i forgot the hover effects hahaha i will add them , thank you so much for your feedback <3

  0
• 

  Abdelilah•180

  @abdeldevprojectsPosted almost 3 years ago

  i uploaded a cleaner version of the project

  0
• 

  Abdelilah•180

  @abdeldevprojectsPosted almost 3 years ago

  Thank you so much for taking time to read my code and writing this amaizing feedback , im going to work today on the things you mentioned , thank you again <3

  0