Milestone: Add mobile menu #13
Conversation
There was a problem hiding this comment.
STATUS: APPROVE ✔️ ✔️ 🍾
Hi @iambenkis
Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉 👏 👏 💯
To highlight 🍾
- All linters are passing ✔️
- Good commit history ✔️
- Good PR description ✔️
- Your README file is professional ✔️
Optional suggestions
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please ping me @tresorsawasawa when you comment so I can receive the notification or use slack with the same name.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
| <header class="header"> | ||
| <!-- The navbar section start--> | ||
| <nav class="nav-bar"> | ||
| <a href="#" class="logo-txt">My Logo</a> | ||
| <i class="fa-solid fa-bars"></i> | ||
| <nav class="nav-bar"> | ||
| <a href="#" class="logo-txt">BenKis</a> | ||
| <i class="fa-solid fa-bars"></i> | ||
| <ul class="nav-list"> | ||
| <li> | ||
| <a href="#projects-container">Portfolio</a> |
There was a problem hiding this comment.
- [ OPTIONAL ] IMPORTANT: I think it would be better if you add a good background color at the header as you decided to make it fixed. This will prevent the header's content to not overlapping the content of other sections when scrolling. I can suggest the same
background colorfor the home section for still following the Figma design
| .active-menu { | ||
| display: block; | ||
| } | ||
|
|
There was a problem hiding this comment.
- [ OPTIONAL ] IMPORTANT: What do you think if you hide the
Scrollingon the document when watching theMobile Menuso this will prevent the user to not interacting with the sub-background content(all sections' content) and your project will now look professional (.i.e: when the user opened the mobile-menu, he can not scroll the content of the sections).
| <section class="menu"> | ||
| <i class="fa-solid fa-xmark"></i> | ||
| <ul class="menu-item"> | ||
| <li> | ||
| <a href="#projects-container" class="meu-item">Portfolio</a> | ||
| </li> | ||
| <li> | ||
| <a href="#about" class="meu-item">About</a> | ||
| </li> | ||
| <li> | ||
| <a href="#contact" class="meu-item">Contact</a> | ||
| </li> | ||
| </ul> | ||
| </section> |
There was a problem hiding this comment.
- [ OPTIONAL ] For good semantics tags, I suggest you wrap all navigation content in a
<nav>tag and not a<section>tag
| <li class="icon"><img src="./images/medium.png" alt="Medium logo"></li> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| </header> | ||
| </div> | ||
| </header> | ||
| <!-- menu section --> | ||
| <section class="menu"> | ||
| <i class="fa-solid fa-xmark"></i> |
There was a problem hiding this comment.
- [ OPTIONAL ] I also think it would be better if you add a
cursor pointerproperty both on theHamburgerandCloseMenubuttons to help users immediately understand that they are clickable.
|
All optional suggestions applied! |
Checklist