NFT preview challenge with Flexbox and pseudo elements

Community feedback

• 

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  👋 Hi Giovanni!

  🎉 Congratulations on finishing your second challenge! Hopefully, this challenge increase your flexbox skill 👌. I really like the eye animation when I hover the equilibrium, so good job on this one!

  I directly notice one odd thing when I see your repo. Your branch name is mainbranch. You don't need to put the word branch, just put the name of your branch, in this case, main.

  More feedback:

  • Accessibility
    • Create a custom :focus-visible styling to any interactive elements (button, links, input, textarea). This will make the users can navigate this website using keyboard (Tab) easily.
    • In this case, any elements that have an active or hover state should be interactive elements (link or button). Try to read about when to use a link or button.
    • For any decorative images, each img tag should have empty alt="" and aria-hidden="true" attributes to make all web assistive technologies such as screen reader ignore those images. In this case, all images are decorative only except the equilibrium and the avatar.
    • For the equilibrium (or dice) image, the alternative text should tell the user what will happen when the user clicks that image. For a better understanding of the alternative text, I would recommend reading the Alt texts: The Ultimate Guide article by Axess Lab and trying to follow this decision tree to decide how to use the alt attribute in various ways.
    • For the avatar image, use the name as the alternative text of his photo. alt="Jules Wyvern"
    • Every page should have one h1. In this case, you can make the Equilibrium #3429 text as a heading.
    • All page content should live inside a landmark (header, main, footer). In this case, you can wrap all page content with main tag except your attribution. For the attribution, swap the div with footer.
    • I would recommend using rem unit as the unit of the body font-size. Using px unit won't allow the user to zoom or scale the page. It's also important to make body font size using relative unit (rem) since most HTML elements inherit the font size of the body element.
  • Styling
    • On landscape mobile view (640px * 360px), the card having full height. You can prevent this by adding padding to the body element.
    • Always make all elements box-sizing: border-box as your common reset. This will make sure that all elements behave the way they should. To learn more, you can search about it.

  html {
    box-sizing: border-box;
  }
  *, *:before, *:after {
    box-sizing: inherit;
  }
  

  • There's no need to set height: 100% on the html element. It's not useful, as far as I know, since the main container or element is the body element, not html.

  That's it! Hopefully, this is helpful!

  Marked as helpful

  2

  ---

  

  Giovanni Beccaro•160

  @GioltekPosted almost 3 years ago

  WOW! @vanzasetia this is a really invaluable feedback! Thank you so much for taking the time to read my code and write all of these suggestions, you are so kind!

  I'm going to answer all of your points:

  -Accessibility

  -- :focus-visible: yes, that's surely something I should start doing, thanks for letting me know! I fixed it on the live website, is it okay now?

  --Fixed the alts as you suggested, they should be fine now! By the way, only the second link you sent me works, the frontend one doesn't for some reason :(

  --The "Equilibrium" title is now a h1!

  --Added the main section and the footer section!

  --Fixed the body font-size! Now it is 1.125rem (which should be 18px according to my google search, which makes sense as 1rem should be 16px).

  -Styling

  --Fixed padding on body so that the card isn't 100% height on mobile!

  --Put your string of file for the box-sizing, next thing I'm gonna do is read about it! I saw someone using it on their code but I never understood it, so I'm definitely gonna look it up!

  --For some reason my html border (I also highlighted it with a border on css to be sure) is way too small, it ends just below the .attribution. If I don't make it 100% height my cardbox won't be centered in the middle of the page, it will just be centered on top of the page for some reason. Do you have any tips about that?

  -Other

  --Thanks for the tip about branches, I definitely should learn github. I just saw a tutorial on how to upload code on it so that I could submit my solution, and the tutorial guy named it "mainbranch" so I just copied it ahah!

  --I'm glad you enjoyed the eye animation! I would love to tweak something here and there in challenges like these, but the main point of them is to create something as much similar as the original layout so I can't do much, but these eye thing was a funny way to experiment. Thanks again for all your tips and sorry if I got something wrong from your points or if I misunderstood something, have a nice day!

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @Gioltek Glad to know that it is helpful for you! 😄

  • Accessibility
    • The current :focus-visible is not as clear (almost the same as the default). I would recommend trying this styling instead and seeing the difference.

  :focus-visible {
      outline: 0.1875rem dotted #00fff7;
      outline-offset: 0.3125rem;
  }
  

  • 😅 Sorry for the second link, here is the correct link.
  • 👍 Good job on fixing all those issues!
  • Styling
    • About height, I would recommend setting min-height: 100vh on body element and removing the height property on html element. Search about vh unit, if don't familiar with it.
  • Additional Feedback
    • I notice one issue, where you can't make h1 as the child of a. It's potentially invalid. Swap it and put the a tag inside the h1.

  Hopefully, this answer all your questions!

  P.S. Generate a new report and see is there any issue left or not.

  1

  

  Giovanni Beccaro•160

  @GioltekPosted almost 3 years ago

  Thanks again for the reply @vanzasetia!

  I tried your :focus visible and yes, it is definitely more clear than before. I was trying to do something easier on the eye, a bit more aesthetic. Would you say that in the case of :focus I should choose legibility over something "prettier" but more difficult to spot?

  100vh worked like a charm, so thanks for the tip! I knew about viewport units but I used them interchangeably with %, I should read more about them so that I know when to use one and when the other. And also thanks for the anchor tag and h1 "incompatibility", I fixed it!

  I also generated a new report just like you suggested and it seems flawless structurely speaking, so thanks for your tips! I hope to find more of your comments in my next challenges, you're really good at this! Have a nice day/good night! :)

  0

  

  Vanza Setia•27,775

  @vanzasetiaPosted almost 3 years ago

  @Gioltek Yes, I would focus on the legibility of the :focus and :focus-visible style, since the purpose is to make sure that the user can navigate the website using keyboard easily.

  Marked as helpful

  0