Community Feedback

• 1

  Hello there, Melissa U! 👋

  Well done on this challenge! 👍 Your solution looks pretty good and is responsive! 🙌 (I like the smooth transition of the mobile navigation.) Good to hear you enjoyed it and are pretty happy with the result! 😀

  A few things I suggest are,

  • Turning the cards in the "OUR CREATIONS" section into links and giving them a hover state would be a nice touch.
  • Turning the social media icons in the footer of the page into actual links using the anchor tag.
  • Perhaps giving the navigation links in the footer of the page a hover state like the links in the header.
  • The text in the "THE LEADER IN INTERACTIVE VR" section begins to overflow the body of the page and create a horizontal scroll bar along the bottom of the page in the desktop layout (at around 1200px and below). You might want to look into that and fix it!

  BTW, if you want to link your JS file to your HTML file in the head of the HTML file, make sure to include the defer attribute in the script tag. This will instruct the browser to load the HTML file and the JS file at the same time but only run the JS file once the HTML file is completely loaded. If you do not include this attribute, the JS might load and try to run before the HTML is loaded (and that will be a problem since there is not HTML for the JS to work with yet). You could also move your script tag to the bottom of the HTML file to resolve this issue, but keeping the script tag in the head of the HTML file and using the defer attribute is slightly faster.

  Hope this helps. 😊

  Keep coding (and happy coding, too)! 😁

  P

  ApplePieGiraffe15,765Posted 1 month ago

  Reply

  • 0

    Thank you for the feedback. I will implement those suggestions. A great explanation for my JS link issue, I completely forgot about the defer attribute.

    P

    Teddiurssa145Posted 1 month ago

    Reply

  Add a comment
• 1

  Great feedback from @ApplePieGiraffe there, as always ☺

  I'm viewing on mobile and notice a couple of things that also need attention:

  • Biggest is that when the menu items scroll back up after closing the mobile menu, some of it stays in view. So I end up with a black bar overlapping the top half of the header
  • align the footer links centrally on mobile. It's like they're sort of centred but text aligned left at the same time right now
  • add some space between those social links in the footer like in the design.

  That's all from me. Really great work on this!

  

  grace-snow7,990Posted 1 month ago

  Reply

  • 1

    Just had an extra look at your menu code. I'm glad you got it working, well done there!

    To improve it you should

    • use interactive elements to trigger the open/close, like a button. Then you get keyboard access and accessibility for free ☺
    • only add event listeners to interactive elements. You should only need a max of 2 on this - for open and close
    • in js consider toggling a class on a parent element like the header instead of adding any inline styles. You can handle all the styling thats needed in css.

    E.g. .nav-list { visibility: hidden; } .menu-open .nav-list { visibility: visible; } See how much cleaner that is? ☺

    It's generally bad practice to be messing with inline styles via js. There are exceptions to that, but its definitely better to use classes for simple dom manipulation like this.

    I hope that's helpful to you!

    

    grace-snow7,990Posted 1 month ago

    Reply
  • 0

    Thank you Grace for the wonderful feedback. I noticed the menu issue when I went to show it to a friend. It works perfectly in chrome and firefox so I will play around with and implement your suggestions. :)

    P

    Teddiurssa145Posted 1 month ago

    Reply

  Add a comment

Give some feedback to Teddiurssa about their solution