Responsive News homepage using Grid

Community feedback

• 

  Cats-n-coffee•670

  @Cats-n-coffeePosted about 2 years ago

  Hi Oubaid!

  Very nice work! Your solution is responsive and you made a nice animation for the mobile menu. To answer your question about clean code (and this will be my personal opinion), you'll find that the code you write tomorrow is cleaner than the code you wrote today, and this will go on and on. That's just because you'll keep learning, reading and getting feedback. I think your code is pretty good on this project. You could look into linters (Eslint probably) everyone uses them. You can add them to VScode (maybe other IDEs as well) or to your projects.

  These are the few comments I have about your project:

  • your Js file uses let but you're not re-assigning the variables, you could use const
  • you have 2 unused variables, you can delete them (once you start using a linter, it'll give you this feedback)
  • the way you added the event handlers to your elements isn't wrong, but nowadays people do this all in Js using addEventListener - you should read up on this
  • since you broke up your CSS into files, you might want to look into Scss or similar, they will allow you to break up your CSS without having to include multiple scripts and do as many requests
  • you forgot the cursor: pointer on the mobile menu and a hover effect on its items
  • maybe your nav could be a header, and the actual menu links be inside the nav
  • you're missing an h1, and your headings hierarchy goes from nothing to h3 (unless I missed something). That's not good for accessibility and might impact SEO (not sure about that). Try not to skip a level. Remember that you can style everything with CSS, so think about this from a page structure POV, not from a styling POV.
  • I think the wrapping article isn't what the tag is for, but maybe you could wrap each individual piece/actual article inside an article?
  • your class names one, two, three, ... should probably be more descriptive. In a bigger project you might have collisions or get confused

  About the footer, I'm not seeing the need for a footer here, unless you include the FEM attribution at the very bottom. I think 3 divs inside a div are fine (you should around/read for about this). A good example for a footer is all the landing pages that have menus, copyright, ... .

  Hope this helps, nice work!

  Marked as helpful

  1

  ---

  

  Oubaid•180

  @oubaidelmoudhikPosted about 2 years ago

  @Cats-n-coffee Thank you very much for the feedback! I did fix everything you talked about, except the class names one two three, because I only named them that to understand my grid, in future project I'll hopefully be more understanding of how grid works. Again, thank you sooo much for your help!

  0