Responsice Space Tourism Website using React and SASS

Community feedback

• 

  Raymart Pamplona•16,100

  @pikapikamartPosted about 3 years ago

  Hey, awesome work on this one. I recently did this challenge as well:>. Desktop layout looks great, site is responsive but the site is not scrollable since there is a overflow: hidden that's why some content are being hidden vertically and can't be seen. Mobile state looks fine but the layout doesn't occupy full height as there is an extra space at the bottom which is still color white.

  Some suggestions would be:

  • Of course removing the overflow: hidden on the body tag so that user will be able to see the full content without the site not being scrollable.
  • 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.
  • It would be great to have:

  <header />
  <main />
  

  • A general rule I think, don't explicitly use vh unit since it is only limited to the current viewport's height. Instead just use rem or use it with min-height and not height.
  • Website-logo-link a tag should have either aria-label attribute or sr-only text inside, that describes where the link would take the user. Usually, website-logo directs user to homepage so use homepage as the value like `aria-label="homepage".
  • Remember that a website-logo is one of the meaningful images on a site so it would be better if you use img to put the website-logo rather than being use as background-image.
  • Remember that interactive components needs to use interactive elements. On the navbar, links are placed so use a tag and certainly not span. Using this makes only your layout not accessible.
  • For each of the navlink's number like 00, you could make something like:

  <a href="">
    <span class="sr-only">00</span>
    Home
  </a>
  

  So that the number won't be read by screen-reader.

  • Avoid using height: 100vh on a large container like the .home 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.
  • so you want to travel to space is a single text so only use an h1 to wrap the text.
  • The explore could be button or a link but not only heading tag.

  VISITING A SINGLE LINK, DESTINATION

  • Remember that for every site page, an h1 should be present. Those text like "pick your destinationcould beh1` since they give information on what the section would contain.
  • Also when using a heading tag, make sure you are not skipping a level. If you use h5 then make sure that h1, h2, h3, h4 are all present.
  • Again for the tab selections for each item, use interactive element and not only li tag. You could use button in here, you could look up my solution for markups that are used.
  • Each images should have their image name as the alt value like alt="Moon" so that it will be more specific rather than using planets.
  • Each destination name could just be h2.
  • Those information below it could use a ul tag to wrap all of them, since those are "list" of information about a destination.

  MOBILE

  • Breakpoint is too small, using only 360px is not enough since there are lots of phones that have higher dimension than that. Also the font-sizes are too small, it would be great if you scale them up.
  • Hamburger menu should be using a button since it is an interactive component.

  SUPPOSING BUTTON IS USED

  • The button should have a default attribute of aria-expanded="false" and it will be set to true when the users toggles it and vice-versa.
  • The hamburger button should have either aria-label attribute or sr-only text inside it which defines what the button does. You could use aria-label="navigation dropdown menu"
  • The dropdown is not being hidden properly. Using only right to transition the element only works visually, but the dropdown is still visible. Instead add visibility: hidden to the default and visibility: visible when the dropdown is toggled. This way, it will be be hidden properly.

  GENERAL

  • Add a max-width somewhere so that the layout won't go out of place. Try zooming out the site, you will notice that elements are being placed differently.

  Aside from those, great job again on this one.

  Marked as helpful

  2

  ---

  

  Fury•945

  @ashiqfuryPosted about 3 years ago

  @pikamart I should appreciate for putting your effort to read my code and spending your valuable time to give these suggestions. I will correct these mistakes.. I learned a lot about your comment. Thank you so much.. :))

  1