Advice-generator-app with - Vanilla JS

Community feedback

• 

  P

  Andreas Remdt•950

  @andreasremdtPosted about 2 years ago

  Hey @Stv-devl,

  Great job finishing the challenge! I went through your solution and code and found it working nicely! There are a couple of improvements I can suggest, feel free to implement them or not :-)

  • The dice button .switch-advice is not a real button, but a div element. This hurts your accessibility, because div elements are not meant to be interactive. Whenever a user should be able to click on something, use either a button for programmable actions (like in this case displaying the next advice), or an a element for visiting another page. Right now, screen reader users wouldn't be able to interact with your app, nor would keyboard-only users since the "button" is not focusable.
  • I like that you used an h1 for the heading, but you don't have to write "ADVICE" in all uppercase. It's better to rely on CSS and text-transform: uppercase. Else, if this page was indexed by any search engine, it would display your page title in uppercase, which you might not want.
  • The separator is an img element with alt="img1" as an attribute. While this works, it can certainly be improved. You could consider using an hr element instead and apply the SVG via background-image, this would be the most semantically correct version. Or, if you want to stick to the img, then it's best set your alt text to alt="" and add aria-hidden="true" as an addtional attribute. The reason being is that screen readers or search engines don't need to care about your separator, it's solely for layout purposes. An image should only have an alt description if it's important content, like the banner image to a blog post. But here, screen readers and other assistive technologies can skip over it.
  • Similarly, your dice button also uses the img element with alt="img2", which is not really accessible. First, "img2" is not a descriptive name, especially blind people can only guess what this image is about without a proper description. Second, your button doesn't have any action label, letting users of assistive technologies clueless to what it does. To fix it, you could add title="Display next advice" to the button and add aria-hidden="true" to the image inside. Screen readers will then read "Display next advice" whenever the user interacts with it.

  I like your JavaScript, it's clean and straight-forward. Also, cool spinner! Hope this helps, let me know if you have any questions :-)

  Marked as helpful

  0

  ---

  

  P

  Stevan-Dev•420

  @Stv-devlPosted about 2 years ago

  @andreasremdt Thank you for Html advice, it's always good to take.

  0