News Homepage

Community feedback

• 

  P

  Jye Harry•250

  @jyeharryPosted about 1 month ago

  Some areas for improvement:

  • I think the nav switches to the mobile nav at a point that's unnecessarily wide. It's still at a decently wide tablet screen where the desktop nav has plenty of room. I'm not saying it's bad at all, just that my personal preference is that I'd make the switch happen at a narrower point.

  • Heading hierarchy could be improved. There can be more than one <h1> on the screen if they're in their own <section>s. Therefore the "New" heading could be a <h1> while the article headings inside that section could be <h2>s. Similarly, the popular article headings could be <h2>s instead of <h3>s. I also used visually hidden <h1> in this section that contained the text "Popular Articles" for accessibility purposes.

  • You could simplify the javascript by using a single button that is relatively positioned and has a high z-index, so that when the mobile nav appears the button actually appears above it, allowing you to click it again. The javascipt then gets simplified by only having to add an event listener to one button and in that event listener there's an if statement that determines whether to add or remove a class. You would also then use the applied class to determine whether the button displays the open or close icon. You could look at my solution to understand what I mean.

  For my solution I took a lot of inspiration from Kevin Powell's solution, especially for the mobile nav. His solution also taught me some uses for aria attributes which are good for accessibility. I think you could learn a from this video like I did, just try not to copy the code as is and try watching it first then seeing if you can do it on your own.

  Marked as helpful

  0

  ---

  

  Luís Sitoe•40

  @luissitoePosted about 1 month ago

  @jyeharry Hey, thanks for the tips, I'll try to improve my solution and I'll watch his video to see If I can get some inspiration too.

  0