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!
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!
@melissaugrai
Posted
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. :)
