Space Tourism - Built With React/ Data.json/ SCSS

Community feedback

• 

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 2 years ago

  Greetings, Steven Portas! 👋

  Amazing job on this challenge! 👏 Your solution looks and feels great and I really like some of the small touches you added (such as the transitions and favicon). 👍

  Just a few tiny suggestions/tips!

  It might be worth adding some headings to pages for accessibility (e.g., such as by making the "01 Pick Your Destination" text a heading) and using button elements for the tabs that change the content of pages to make your HTML more semantic (as this is typically recommended over trying to make elements like divs or spans interactive).

  Adding an aria-label to the mobile menu button would be a good idea since there isn't any text inside of it to identify it.

  Also, you don't need to add "Image of..." in your alt text for images, as screen readers will announce that they have found an image before proceeding to the read the alt text.

  The logo doesn't need an aria-label since it is an image element and already has alt text. That aria-label would be better placed on the a element itself. 😉

  Hope you find these tips helpful. 😊

  Keep coding (and happy coding, too)! 😁

  Marked as helpful

  1

  ---

  

  Steven Portas•440

  @satropPosted almost 2 years ago

  @ApplePieGiraffe awesome! I totally space on the ADA in a few places because I was over concentrating on fixing the routing issue with React and gh-pages that I forgot to do my accessibility pass! But I really do appreciate you looking closely enough to point that out, I’ll go back and fix them.

  As for the buttons not being button I was positive that SwiperJS was taking care of that. So I’ll have a look at that as well.

  1

  

  Steven Portas•440

  @satropPosted almost 2 years ago

  @ApplePieGiraffe Just this second went back in and made all but one update that you recommended.

  I didn't change the spans to button elements. I went back and had a wee look and I thought SwiperJS was doing something to this situation and I was right. The spans have an added role attribute of "button". So that will make sure it's more in tune with semantics.

  <span class="swiper-pagination-bullet swiper-pagination-bullet-active" tabindex="0" role="button" aria-label="Go to slide 1" aria-current="true"></span>
  

  1

  

  T

  ApplePieGiraffe•30,545

  @ApplePieGiraffePosted almost 2 years ago

  @satrop

  Haha, I often do the same thing! It's easy to get caught up in JS sometimes. 😆 Glad that helped! 👍

  0