Room homepage with sass

Community feedback

• 

  T

  Grace•31,230

  @grace-snowPosted almost 4 years ago

  Hi Mills,

  This looks OK on mobile but isn't quite matching the design in some places like image height and nav styles.

  I can spot a few issues with the html that could do with some attention too:

  • interactive elements need to use interactive html tags like links or buttons
  • you can only have one h1 per page. It's not right to have the logo as a h1, but it is hard to know what to do with this site as its not clear what the h1 should be due to the slider content. I'd be tempted to add a hidden h1 to the page, or make the title of the first slide a h1 and nothing else
  • as well as being an interactive element, the hamburger needs to clearly link to the menu with attributes like aria-controls and aria-expanded. And it should sit within the nav element.
  • you could use the rel=next on the next button (link) and similar for the previous button for better semantics too

  Your js looks pretty good to me. A best practice tip for future would be to get into the habit of always selecting elements with a consistent class syntax. Eg I like to use js- prefixed class names for anything I'm going to select in javascript, so it's really obvious what that class is doing. It's much harder if you're using a mixture of classes and ids and element selectors in your js, and once you're working in a team these can cause problems as other devs are more likely to change those things in the html. If they see a js- class there they won't change it.

  Hope thats helpful advice. Keep going ☺

  2

  ---

  

  Milos•330

  @MilosSimic994Posted almost 4 years ago

  @grace-snow

  Grace, thank you very much for such detailed feedback, your advice means a lot to me, I will do my best to correct all of the above and I expect to get a totally different feedback on my next project :)

  0